You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ariatosca.apache.org by tliron <gi...@git.apache.org> on 2017/10/31 20:45:11 UTC
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
GitHub user tliron opened a pull request:
https://github.com/apache/incubator-ariatosca/pull/207
ARIA-1 Parser test suite
This commit additionally fixes many parser bugs revealed by the test
suite, which includes adding validations that were missing.
A new "extensions" tox suite is introduced.
The /tests/parser cases were refactored into /tests/topology and
/tests/extensions.
The Hello World example was fixed and refactored, as it in fact had
invalid TOSCA (it previously passed due to a missing validation).
Parser performance was greatly improved by:
1. Switching to the YAML C library
2. Aggressive caching of parsed presentations
3. The ability to skip importing the TOSCA profile
4. A new deepcopy_fast util
5. A new BlockingExecutor that is much faster for single-threaded use
Unicode is now fully supported for all validation and log message. This
requires the use a unicode (u'' notation) for all .format specs.
Additionally, PyLint comment directives have been standardized by
pushing them to column 100.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-ariatosca ARIA-1-parser-test-suite
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-ariatosca/pull/207.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #207
----
commit 22cee6d90c5873230eabe7449b5bbccf33222bad
Author: Tal Liron <ta...@gmail.com>
Date: 2017-08-17T22:50:27Z
ARIA-1 Parser test suite
This commit additionally fixes many parser bugs revealed by the test
suite, which includes adding validations that were missing.
A new "extensions" tox suite is introduced.
The /tests/parser cases were refactored into /tests/topology and
/tests/extensions.
The Hello World example was fixed and refactored, as it in fact had
invalid TOSCA (it previously passed due to a missing validation).
Parser performance was greatly improved by:
1. Switching to the YAML C library
2. Aggressive caching of parsed presentations
3. The ability to skip importing the TOSCA profile
4. A new deepcopy_fast util
5. A new BlockingExecutor that is much faster for single-threaded use
Unicode is now fully supported for all validation and log message. This
requires the use a unicode (u'' notation) for all .format specs.
Additionally, PyLint comment directives have been standardized by
pushing them to column 100.
----
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153343290
--- Diff: aria/utils/threading.py ---
@@ -161,11 +242,7 @@ def close(self):
self._workers = None
def drain(self):
- """
- Blocks until all current tasks finish execution, but leaves the worker threads alive.
- """
-
- self._tasks.join() # oddly, the API does not support a timeout parameter
+ self._tasks.join() # oddly, the API does not support a timeout parameter
--- End diff --
Sure, but for better or for worse it's what we have right now. I did enough sweeping fixes in this PR to get people mad at me, I suggest postponing this one for a different PR.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153008880
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
+ def __init__(self, presentation, canonical_location, origin_canonical_location):
+ self.presentation = presentation
+ self.canonical_location = canonical_location
+ self.origin_canonical_location = origin_canonical_location
+ self.merged = False
+
+ def get_imports(self, results):
+ imports = []
+
+ def has_import(result):
+ for i in imports:
+ if i.canonical_location == result.canonical_location:
+ return True
+ return False
+
+ for result in results:
+ if result.origin_canonical_location == self.canonical_location:
+ if not has_import(result):
+ imports.append(result)
+ return imports
+
+ def merge(self, results, context):
+ # Make sure to only merge each presentation once
+ if self.merged:
+ return
+ self.merged = True
+ for result in results:
+ if result.presentation == self.presentation:
+ result.merged = True
+
+ for result in self.get_imports(results):
+ # Make sure import is merged
+ result.merge(results, context)
+
+ # Validate import
+ if hasattr(self.presentation, '_validate_import'):
+ if not self.presentation._validate_import(context, result.presentation):
+ # _validate_import will report an issue if invalid
+ continue
+
+ # Merge import
+ if hasattr(self.presentation, '_merge_import'):
+ self.presentation._merge_import(result.presentation)
+
+ def cache(self):
+ if not self.merged:
+ raise RuntimeError(u'Only merged presentations can be cached: {0}'
+ .format(self.canonical_location))
+ PRESENTATION_CACHE[self.canonical_location] = self.presentation
+
+
+class _Skip(Exception):
--- End diff --
+1 I renamed it to "_CancelPresentation"
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153014247
--- Diff: aria/parser/presentation/presentation.py ---
@@ -199,6 +199,9 @@ class Presentation(PresentationBase):
"""
def _validate(self, context):
+ if (not context.presentation.configuration.get('validate_normative', True)) \
--- End diff --
+1
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150774520
--- Diff: aria/parser/consumption/presentation.py ---
@@ -31,47 +32,33 @@ class Read(Consumer):
instances.
It supports agnostic raw data composition for presenters that have
- ``_get_import_locations`` and ``_merge_import``.
+ ``_get_import_locations``, ``_validate_import``, and ``_merge_import``.
To improve performance, loaders are called asynchronously on separate threads.
Note that parsing may internally trigger more than one loading/reading/presentation
cycle, for example if the agnostic raw data has dependencies that must also be parsed.
"""
- def consume(self):
- if self.context.presentation.location is None:
- self.context.validation.report('Presentation consumer: missing location')
- return
-
- presenter = None
- imported_presentations = None
+ def __init__(self, context):
+ super(Read, self).__init__(context)
+ self._cache = {}
- executor = FixedThreadPoolExecutor(size=self.context.presentation.threads,
- timeout=self.context.presentation.timeout)
- executor.print_exceptions = self.context.presentation.print_exceptions
- try:
- presenter = self._present(self.context.presentation.location, None, None, executor)
- executor.drain()
-
- # Handle exceptions
- for e in executor.exceptions:
- self._handle_exception(e)
+ def consume(self):
+ # Present the main location and all imports recursively
+ main, results = self._present_all()
--- End diff --
what does `main` and 'results' stand for? `main_service_template` and `imported_service_templates`? It's not really clear
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150822870
--- Diff: tests/mechanisms/utils.py ---
@@ -0,0 +1,71 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import itertools
+
+
+def matrix(*iterables, **kwargs):
+ """
+ Generates a matrix of parameters for ``@pytest.mark.parametrize``.
--- End diff --
why counts is hidden inside kwargs?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150818105
--- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/data_types.py ---
@@ -318,15 +318,15 @@ def report(message, constraint):
#
def get_data_type_value(context, presentation, field_name, type_name):
- the_type = get_type_by_name(context, type_name, 'data_types')
- if the_type is not None:
- value = getattr(presentation, field_name)
- if value is not None:
+ value = getattr(presentation, field_name)
+ if value is not None:
+ the_type = get_type_by_name(context, type_name, 'data_types')
--- End diff --
naming...
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153012904
--- Diff: tests/mechanisms/parsing/__init__.py ---
@@ -0,0 +1,75 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pytest
+import jinja2
+
+
+LINE_BREAK = '\n' + '-' * 60
+
+
+class Parsed(object):
+ def __init__(self):
+ self.issues = []
+ self.text = ''
+ self.verbose = False
+
+ def assert_success(self):
+ __tracebackhide__ = True # pylint: disable=unused-variable
--- End diff --
A PyTest feature ... I will link to the documentation.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153009028
--- Diff: aria/parser/presentation/context.py ---
@@ -63,3 +67,12 @@ def get_from_dict(self, *names):
"""
return self.presenter._get_from_dict(*names) if self.presenter is not None else None
+
+ def create_executor(self):
+ if self.threads == 1:
--- End diff --
What do you mean by "initiator"? You can configure the thread count in the parser context, just like everything else, whenever you start the parsing process. Normally you don't need to do this -- the defaults should be fine. Just for running tests in tox (which is multiprocess) it makes sense to override the default and enforce single-threading.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153014161
--- Diff: aria/parser/presentation/field_validators.py ---
@@ -14,12 +14,29 @@
# limitations under the License.
+from ...utils.formatting import safe_repr
from ..validation import Issue
from .utils import (parse_types_dict_names, report_issue_for_unknown_type,
report_issue_for_parent_is_self, report_issue_for_unknown_parent_type,
report_issue_for_circular_type_hierarchy)
+def not_negative_validator(field, presentation, context):
--- End diff --
Python will do the right thing here -- if there is a `__lt__` it will call it, if not it will try to find a `__gt__` and `__eq__` and do the right stuff.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151400932
--- Diff: aria/parser/presentation/presentation.py ---
@@ -199,6 +199,9 @@ class Presentation(PresentationBase):
"""
def _validate(self, context):
+ if (not context.presentation.configuration.get('validate_normative', True)) \
--- End diff --
This is the mechanism that skips validating normative types? I think an explanation should be added to this condition.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150777791
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
--- End diff --
Not used anywhere
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153012524
--- Diff: tests/extensions/aria_extension_tosca/simple_v1_0/functions/test_function_concat.py ---
@@ -0,0 +1,102 @@
+ # -*- coding: utf-8 -*-
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+def test_functions_concat_syntax_empty(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [] }
+""").assert_success()
+
+
+def test_functions_concat_strings(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [ a, b, c ] }
+""").assert_success()
+
+
+def test_functions_concat_mixed(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [ a, 1, 1.1, null, [], {} ] }
+""").assert_success()
+
+
+def test_functions_concat_nested(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [ a, { concat: [ b, c ] } ] }
+""").assert_success()
+
+
+# Unicode
+
+def test_functions_concat_unicode(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ 類型:
+ properties:
+ 參數:
+ type: string
+topology_template:
+ node_templates:
+ 模板:
+ type: 類型
+ properties:
+ 參數: { concat: [ 一, 二, 三 ] }
+""").assert_success()
--- End diff --
It would impossible to create such a test that would work with non-TOSCA parsers.
In the future we will boost our topology engine tests to test actual function evaluation. )And of course things get much more complicated there, because we deal with HOST, TARGET, and other topological aspects, as well as runtime values in `get_attribute`.)
For this suite, we are just testing parsing of intrinsic functions, not their evaluation.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153011389
--- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
@@ -139,9 +139,18 @@ def get_template_capabilities(context, presentation):
if values:
capability_assignment._raw['properties'] = values
capability_assignment._reset_method_cache()
+
--- End diff --
It's explained in the `__init__.py` of the package:
```
"""
Creates ARIA service template models based on the TOSCA presentation.
Relies on many helper methods in the presentation classes.
"""
```
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150785935
--- Diff: aria/parser/reading/yaml.py ---
@@ -16,18 +16,30 @@
from .reader import Reader
from .locator import Locator
from .exceptions import ReaderSyntaxError
-from .locator import LocatableString, LocatableInt, LocatableFloat
+from .locator import (LocatableString, LocatableInt, LocatableFloat)
-# Add our types to ruamel.yaml
+
+MERGE_TAG = u'tag:yaml.org,2002:merge'
+MAP_TAG = u'tag:yaml.org,2002:map'
+
+
+# Add our types to RoundTripRepresenter
yaml.representer.RoundTripRepresenter.add_representer(
LocatableString, yaml.representer.RoundTripRepresenter.represent_unicode)
yaml.representer.RoundTripRepresenter.add_representer(
LocatableInt, yaml.representer.RoundTripRepresenter.represent_int)
yaml.representer.RoundTripRepresenter.add_representer(
LocatableFloat, yaml.representer.RoundTripRepresenter.represent_float)
-MERGE_TAG = u'tag:yaml.org,2002:merge'
-MAP_TAG = u'tag:yaml.org,2002:map'
+
+def construct_yaml_map(self, node):
--- End diff --
add documentation as to why and how :)
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153007941
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
--- End diff --
Yes there is. An "if" statement plus a retrieval statement are non-atomic, and since we are in a concurrent situation it's possible that between the if and the retrieval that the data will removed from the cache, causing the retrieval to fail with an exception even though the "if" succeeded. A single retrieval is atomic.
(This is the generally the idiomatic "Python way" to do this -- always choose the atomic!)
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151404126
--- Diff: aria/parser/reading/yaml.py ---
@@ -16,18 +16,30 @@
from .reader import Reader
from .locator import Locator
from .exceptions import ReaderSyntaxError
-from .locator import LocatableString, LocatableInt, LocatableFloat
+from .locator import (LocatableString, LocatableInt, LocatableFloat)
-# Add our types to ruamel.yaml
+
+MERGE_TAG = u'tag:yaml.org,2002:merge'
+MAP_TAG = u'tag:yaml.org,2002:map'
+
+
+# Add our types to RoundTripRepresenter
yaml.representer.RoundTripRepresenter.add_representer(
LocatableString, yaml.representer.RoundTripRepresenter.represent_unicode)
yaml.representer.RoundTripRepresenter.add_representer(
LocatableInt, yaml.representer.RoundTripRepresenter.represent_int)
yaml.representer.RoundTripRepresenter.add_representer(
LocatableFloat, yaml.representer.RoundTripRepresenter.represent_float)
-MERGE_TAG = u'tag:yaml.org,2002:merge'
-MAP_TAG = u'tag:yaml.org,2002:map'
+
+def construct_yaml_map(self, node):
--- End diff --
(even though this function existed before =)
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150818011
--- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
@@ -139,9 +139,18 @@ def get_template_capabilities(context, presentation):
if values:
capability_assignment._raw['properties'] = values
capability_assignment._reset_method_cache()
+
--- End diff --
package requires explaining
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150822303
--- Diff: tests/mechanisms/parsing/__init__.py ---
@@ -0,0 +1,75 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pytest
+import jinja2
+
+
+LINE_BREAK = '\n' + '-' * 60
+
+
+class Parsed(object):
+ def __init__(self):
+ self.issues = []
+ self.text = ''
+ self.verbose = False
+
+ def assert_success(self):
+ __tracebackhide__ = True # pylint: disable=unused-variable
--- End diff --
?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151080284
--- Diff: aria/__init__.py ---
@@ -47,22 +46,19 @@
def install_aria_extensions(strict=True):
"""
- Iterates all Python packages with names beginning with ``aria_extension_`` and all
- ``aria_extension`` entry points and loads them.
-
- It then invokes all registered extension functions.
+ Loads all Python packages with names beginning with ``aria_extension_`` and calls their
+ ``aria_extension`` initialization entry points if they have them.
- :param strict: if set to ``True``, Tries to load extensions with
- dependency versions under consideration. Otherwise tries to load the
- required package without version consideration. Defaults to True.
+ :param strict: if ``True`` tries to load extensions while taking into account the versions
+ of their dependencies, otherwise ignores versions
:type strict: bool
"""
for loader, module_name, _ in iter_modules():
if module_name.startswith('aria_extension_'):
loader.find_module(module_name).load_module(module_name)
for entry_point in pkg_resources.iter_entry_points(group='aria_extension'):
# It should be possible to enable non strict loading - use the package
- # that is already installed inside the environment, and forgo the
+ # that is already installed inside the environment, and forego the
--- End diff --
forgo
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153151961
--- Diff: aria/parser/loading/loader.py ---
@@ -32,3 +32,6 @@ def close(self):
def load(self):
raise NotImplementedError
+
+ def get_canonical_location(self): # pylint: disable=no-self-use
--- End diff --
That every method definition that is just used to 'declare' it to the inheriting classes should return `NotImplementedError`. and overriding methods should do what they want to do.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153014666
--- Diff: aria/utils/threading.py ---
@@ -161,11 +242,7 @@ def close(self):
self._workers = None
def drain(self):
- """
- Blocks until all current tasks finish execution, but leaves the worker threads alive.
- """
-
- self._tasks.join() # oddly, the API does not support a timeout parameter
+ self._tasks.join() # oddly, the API does not support a timeout parameter
--- End diff --
Hm, I always hated this, and a lot of projects ignore it. Right now we have a mix of styles.
Let's put this as a separate JIRA and maybe change all our comments at once?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153011299
--- Diff: examples/hello-world/hello-world.yaml ---
@@ -2,30 +2,24 @@ tosca_definitions_version: tosca_simple_yaml_1_0
node_types:
- WebServer:
- derived_from: tosca:Root
- capabilities:
- host:
- type: tosca:Container
-
- WebApp:
+ HelloWorld:
--- End diff --
Where?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153008578
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
+ def __init__(self, presentation, canonical_location, origin_canonical_location):
+ self.presentation = presentation
+ self.canonical_location = canonical_location
+ self.origin_canonical_location = origin_canonical_location
+ self.merged = False
+
+ def get_imports(self, results):
+ imports = []
+
+ def has_import(result):
+ for i in imports:
+ if i.canonical_location == result.canonical_location:
+ return True
+ return False
+
+ for result in results:
+ if result.origin_canonical_location == self.canonical_location:
+ if not has_import(result):
+ imports.append(result)
+ return imports
+
+ def merge(self, results, context):
+ # Make sure to only merge each presentation once
+ if self.merged:
+ return
+ self.merged = True
+ for result in results:
+ if result.presentation == self.presentation:
+ result.merged = True
+
+ for result in self.get_imports(results):
+ # Make sure import is merged
+ result.merge(results, context)
+
+ # Validate import
+ if hasattr(self.presentation, '_validate_import'):
--- End diff --
As above.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151398089
--- Diff: aria/parser/presentation/field_validators.py ---
@@ -14,12 +14,29 @@
# limitations under the License.
+from ...utils.formatting import safe_repr
from ..validation import Issue
from .utils import (parse_types_dict_names, report_issue_for_unknown_type,
report_issue_for_parent_is_self, report_issue_for_unknown_parent_type,
report_issue_for_circular_type_hierarchy)
+def not_negative_validator(field, presentation, context):
--- End diff --
Maybe you should use `__gt__()` and `<` instead. I've seen somewhere else that you replaced a custom equality method with an override of `__eq__()`.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151401197
--- Diff: aria/parser/presentation/presentation.py ---
@@ -199,6 +199,9 @@ class Presentation(PresentationBase):
"""
def _validate(self, context):
+ if (not context.presentation.configuration.get('validate_normative', True)) \
+ and self._get_extension('normative'):
+ return
--- End diff --
In addition, do we have any method of detecting if the user is using incorrect normative types? For example, he references a file from within their company and that file has a typo.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150774246
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
+ def __init__(self, presentation, canonical_location, origin_canonical_location):
+ self.presentation = presentation
+ self.canonical_location = canonical_location
+ self.origin_canonical_location = origin_canonical_location
+ self.merged = False
+
+ def get_imports(self, results):
+ imports = []
+
+ def has_import(result):
+ for i in imports:
+ if i.canonical_location == result.canonical_location:
+ return True
+ return False
+
+ for result in results:
+ if result.origin_canonical_location == self.canonical_location:
+ if not has_import(result):
+ imports.append(result)
+ return imports
+
+ def merge(self, results, context):
+ # Make sure to only merge each presentation once
+ if self.merged:
+ return
+ self.merged = True
+ for result in results:
+ if result.presentation == self.presentation:
+ result.merged = True
+
+ for result in self.get_imports(results):
+ # Make sure import is merged
+ result.merge(results, context)
+
+ # Validate import
+ if hasattr(self.presentation, '_validate_import'):
+ if not self.presentation._validate_import(context, result.presentation):
+ # _validate_import will report an issue if invalid
+ continue
+
+ # Merge import
+ if hasattr(self.presentation, '_merge_import'):
+ self.presentation._merge_import(result.presentation)
+
+ def cache(self):
+ if not self.merged:
+ raise RuntimeError(u'Only merged presentations can be cached: {0}'
+ .format(self.canonical_location))
+ PRESENTATION_CACHE[self.canonical_location] = self.presentation
+
+
+class _Skip(Exception):
--- End diff --
consider naming it SelfImportException
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150782911
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
--- End diff --
maybe add default implementation for `_link_locators` and skip the if
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013913
--- Diff: aria/parser/loading/loader.py ---
@@ -32,3 +32,6 @@ def close(self):
def load(self):
raise NotImplementedError
+
+ def get_canonical_location(self): # pylint: disable=no-self-use
--- End diff --
What do you propose?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150786459
--- Diff: aria/utils/collections.py ---
@@ -220,27 +225,30 @@ def __setitem__(self, key, value, **_):
return super(StrictDict, self).__setitem__(key, value)
-def merge(dict_a, dict_b, path=None, strict=False):
+def merge(dict_a, dict_b, copy=True, strict=False, path=None):
--- End diff --
add documentation explaining the args...
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013674
--- Diff: aria/parser/consumption/presentation.py ---
@@ -13,15 +13,16 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-
-from ...utils.threading import FixedThreadPoolExecutor
-from ...utils.formatting import json_dumps, yaml_dumps
+from ...utils.formatting import (json_dumps, yaml_dumps)
from ..loading import UriLocation
-from ..reading import AlreadyReadException
from ..presentation import PresenterNotFoundError
from .consumer import Consumer
+PRESENTATION_CACHE = {}
+CANONICAL_LOCATION_CACHE = {}
+
+
class Read(Consumer):
--- End diff --
+1
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153344804
--- Diff: aria/parser/loading/uri.py ---
@@ -44,6 +45,7 @@ def __init__(self, context, location, origin_location=None):
self.location = location
self._prefixes = StrictList(value_class=basestring)
self._loader = None
+ self._canonical_location = None
--- End diff --
I added documentation in `loader.py` (the base class).
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151405515
--- Diff: aria/parser/reading/yaml.py ---
@@ -82,7 +84,11 @@ def read(self):
# see issue here:
# https://bitbucket.org/ruamel/yaml/issues/61/roundtriploader-causes-exceptions-with
#yaml_loader = yaml.RoundTripLoader(data)
- yaml_loader = yaml.SafeLoader(data)
+ try:
+ # Faster C-based loader, might not be available on all platforms
+ yaml_loader = yaml.CSafeLoader(data)
+ except BaseException:
--- End diff --
`BaseException` will catch every exception, even `SystemExit` and `KeyboardInterrupt`. Are you sure?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153006422
--- Diff: extensions/aria_extension_tosca/simple_v1_0/assignments.py ---
@@ -144,6 +144,17 @@ def _get_type(self, context):
# In RelationshipAssignment
the_type = the_type[0] # This could be a RelationshipTemplate
+ if isinstance(self._container._container, RequirementAssignment):
--- End diff --
+1 refactored
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153150480
--- Diff: aria/utils/threading.py ---
@@ -93,7 +92,104 @@ def sum(arg1, arg2):
print executor.returns
"""
- _CYANIDE = object() # Special task marker used to kill worker threads.
+ def __init__(self, print_exceptions=False):
+ self.print_exceptions = print_exceptions
+
+ def submit(self, func, *args, **kwargs):
+ """
+ Submit a task for execution.
+
+ The task will be called ASAP on the next available worker thread in the pool.
+
+ :raises ExecutorException: if cannot be submitted
+ """
+ raise NotImplementedError
+
+ def close(self):
+ """
+ Blocks until all current tasks finish execution and all worker threads are dead.
+
+ You cannot submit tasks anymore after calling this.
+
+ This is called automatically upon exit if you are using the ``with`` keyword.
+ """
+ pass
+
+ def drain(self):
+ """
+ Blocks until all current tasks finish execution, but leaves the worker threads alive.
+ """
+ pass
+
+ @property
+ def returns(self):
+ """
+ The returned values from all tasks, in order of submission.
+ """
+ return ()
+
+ @property
+ def exceptions(self):
+ """
+ The raised exceptions from all tasks, in order of submission.
+ """
+ return ()
+
+ def raise_first(self):
+ """
+ If exceptions were thrown by any task, then the first one will be raised.
+
+ This is rather arbitrary: proper handling would involve iterating all the exceptions.
+ However, if you want to use the "raise" mechanism, you are limited to raising only one of
+ them.
+ """
+
+ exceptions = self.exceptions
+ if exceptions:
+ raise exceptions[0]
+
+ def __enter__(self):
+ return self
+
+ def __exit__(self, the_type, value, traceback):
+ pass
+
+
+class BlockingExecutor(Executor):
+ """
+ Executes tasks in the current thread.
+ """
+
+ def __init__(self, print_exceptions=False):
+ super(BlockingExecutor, self).__init__(print_exceptions=print_exceptions)
--- End diff --
I reread the code, and I agree.
---
[GitHub] incubator-ariatosca issue #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/207
BTW please don't squash your commits...
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153009232
--- Diff: aria/parser/presentation/field_validators.py ---
@@ -14,12 +14,29 @@
# limitations under the License.
+from ...utils.formatting import safe_repr
from ..validation import Issue
from .utils import (parse_types_dict_names, report_issue_for_unknown_type,
report_issue_for_parent_is_self, report_issue_for_unknown_parent_type,
report_issue_for_circular_type_hierarchy)
+def not_negative_validator(field, presentation, context):
+ """
+ Makes sure that the field is not negative.
+
+ Can be used with the :func:`field_validator` decorator.
+ """
+
+ field.default_validate(presentation, context)
+ value = getattr(presentation, field.name)
+ if (value is not None) and (value < 0):
--- End diff --
The field also has a type which is enforced as a separate validation. I wanted this particular validator to be general purpose, so it would work with any kind of object that supports comparison ("__gt__" and other magic methods).
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013659
--- Diff: aria/__init__.py ---
@@ -47,22 +46,19 @@
def install_aria_extensions(strict=True):
"""
- Iterates all Python packages with names beginning with ``aria_extension_`` and all
- ``aria_extension`` entry points and loads them.
-
- It then invokes all registered extension functions.
+ Loads all Python packages with names beginning with ``aria_extension_`` and calls their
+ ``aria_extension`` initialization entry points if they have them.
- :param strict: if set to ``True``, Tries to load extensions with
- dependency versions under consideration. Otherwise tries to load the
- required package without version consideration. Defaults to True.
+ :param strict: if ``True`` tries to load extensions while taking into account the versions
+ of their dependencies, otherwise ignores versions
:type strict: bool
"""
for loader, module_name, _ in iter_modules():
if module_name.startswith('aria_extension_'):
loader.find_module(module_name).load_module(module_name)
for entry_point in pkg_resources.iter_entry_points(group='aria_extension'):
# It should be possible to enable non strict loading - use the package
- # that is already installed inside the environment, and forgo the
+ # that is already installed inside the environment, and forego the
--- End diff --
+1
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153148567
--- Diff: aria/parser/reading/yaml.py ---
@@ -82,7 +84,11 @@ def read(self):
# see issue here:
# https://bitbucket.org/ruamel/yaml/issues/61/roundtriploader-causes-exceptions-with
#yaml_loader = yaml.RoundTripLoader(data)
- yaml_loader = yaml.SafeLoader(data)
+ try:
+ # Faster C-based loader, might not be available on all platforms
+ yaml_loader = yaml.CSafeLoader(data)
+ except BaseException:
--- End diff --
so why no `except Exception`?
In this case, you won't ignore a shutdown or an interrupt signal, but any 'programmatic' error will be caught.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r149922174
--- Diff: tests/extensions/aria_extension_tosca/simple_nfv_v1_0/test_profile.py ---
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
--- End diff --
Maybe i don't yet understand the entire testing suite, but all of the tests test the full path. I think i need an introduction into the new parser testing mechanism
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153008025
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
--- End diff --
+1 removed
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151185309
--- Diff: aria/parser/consumption/presentation.py ---
@@ -31,47 +32,33 @@ class Read(Consumer):
instances.
It supports agnostic raw data composition for presenters that have
- ``_get_import_locations`` and ``_merge_import``.
+ ``_get_import_locations``, ``_validate_import``, and ``_merge_import``.
To improve performance, loaders are called asynchronously on separate threads.
Note that parsing may internally trigger more than one loading/reading/presentation
cycle, for example if the agnostic raw data has dependencies that must also be parsed.
"""
- def consume(self):
- if self.context.presentation.location is None:
- self.context.validation.report('Presentation consumer: missing location')
- return
-
- presenter = None
- imported_presentations = None
+ def __init__(self, context):
+ super(Read, self).__init__(context)
+ self._cache = {}
- executor = FixedThreadPoolExecutor(size=self.context.presentation.threads,
- timeout=self.context.presentation.timeout)
- executor.print_exceptions = self.context.presentation.print_exceptions
- try:
- presenter = self._present(self.context.presentation.location, None, None, executor)
- executor.drain()
-
- # Handle exceptions
- for e in executor.exceptions:
- self._handle_exception(e)
+ def consume(self):
+ # Present the main location and all imports recursively
+ main, results = self._present_all()
- imported_presentations = executor.returns
- finally:
- executor.close()
+ # Merge presentations
+ main.merge(results, self.context)
- # Merge imports
- if (imported_presentations is not None) and hasattr(presenter, '_merge_import'):
- for imported_presentation in imported_presentations:
- okay = True
- if hasattr(presenter, '_validate_import'):
- okay = presenter._validate_import(self.context, imported_presentation)
- if okay:
- presenter._merge_import(imported_presentation)
+ # Cache merged presentations
+ if self.context.presentation.cache:
+ for result in results:
+ result.cache()
- self.context.presentation.presenter = presenter
+ self.context.presentation.presenter = main.presentation
+ if main.canonical_location is not None:
--- End diff --
when does main does not a a canonical location?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150778829
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
--- End diff --
Is there a reason not to use `canonical_location in PRESENTATION_CACHE` instead of try and except?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r149718206
--- Diff: tests/extensions/aria_extension_tosca/simple_nfv_v1_0/test_profile.py ---
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
--- End diff --
You are looking here specifically at the test to make sure the NFV profile is valid. This is not where the unit tests for the parser lie.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151187582
--- Diff: aria/parser/loading/loader.py ---
@@ -32,3 +32,6 @@ def close(self):
def load(self):
raise NotImplementedError
+
+ def get_canonical_location(self): # pylint: disable=no-self-use
--- End diff --
Seeing that this is the 'base' Loader class, I know that a lot of time we just declare methods here that raise `NotImplementedError` or just `pass`. Assuming that this is a way to 'declare' these methods to the child classes, I understand its need. But combined with a method that returns `None`, where this `Loader` class is not intended for direct use, is confusing.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150820840
--- Diff: tests/extensions/aria_extension_tosca/simple_v1_0/functions/test_function_get_artifact.py ---
@@ -0,0 +1,156 @@
+# -*- coding: utf-8 -*-
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pytest
+
+
+# Syntax
+
+def test_functions_get_artifact_syntax_empty(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { get_artifact: [] } # needs at least two args
+""").assert_failure()
+
+
+# Arguments
+
+def test_functions_get_artifact_2_arguments(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+artifact_types:
+ MyType: {}
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+ artifacts:
+ my_artifact:
+ type: MyType
+ file: filename
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { get_artifact: [ my_node, my_artifact ] }
+""").assert_success()
--- End diff --
again, check the correct artifact was set
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151402533
--- Diff: aria/parser/reading/reader.py ---
@@ -28,16 +28,9 @@ def __init__(self, context, location, loader):
def load(self):
with OpenClose(self.loader) as loader:
--- End diff --
Couldn't you just made the `Loader` class to be able to be a context manager? What was the need to add another layer of abstraction?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r149671678
--- Diff: tests/extensions/aria_extension_tosca/simple_nfv_v1_0/test_profile.py ---
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
--- End diff --
So basically we have no unittests for the parser
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r149667052
--- Diff: aria/parser/consumption/presentation.py ---
@@ -13,65 +13,52 @@
# See the License for the specific language governing permissions and
--- End diff --
Only over Zoom code review for any parser related modules
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r149669453
--- Diff: extensions/aria_extension_tosca/simple_v1_0/assignments.py ---
@@ -144,6 +144,17 @@ def _get_type(self, context):
# In RelationshipAssignment
the_type = the_type[0] # This could be a RelationshipTemplate
+ if isinstance(self._container._container, RequirementAssignment):
--- End diff --
alot of cascading ifs, consider using `if not isinstance(self._container._container, RequirementAssignment): return False` etc...
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150822924
--- Diff: tests/mechanisms/utils.py ---
@@ -0,0 +1,71 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import itertools
+
+
+def matrix(*iterables, **kwargs):
+ """
+ Generates a matrix of parameters for ``@pytest.mark.parametrize``.
+
+ The matrix is essentially the Cartesian product of the arguments (which should be iterables),
+ with the added ability to "flatten" each value by breaking up tuples and recombining them into a
+ final flat value.
+
+ To do such recombination, use the ``counts`` argument (tuple) to specify the number of elements
+ per value in order. Any count greater than 1 (the default) enables recombination of that value.
+
+ Example::
+
+ x = ('hello', 'goodbye')
+ y = ('Linus', 'Richard')
+ matrix(x, y) ->
+ ('hello', 'Linus'),
+ ('hello', 'Richard'),
+ ('goodbye', 'Linus'),
+ ('goodbye', 'Richard')
+
+ y = (('Linus', 'Torvalds'), ('Richard', 'Stallman'))
+ matrix(x, y) ->
+ ('hello', ('Linus', 'Torvalds')),
+ ('hello', ('Richard', 'Stallman')),
+ ('goodbye', ('Linus', 'Torvalds')),
+ ('goodbye', ('Richard', 'Stallman'))
+
+ matrix(x, y, counts=(1, 2)) ->
+ ('hello', 'Linus', 'Torvalds'),
+ ('hello', 'Richard', 'Stallman'),
+ ('goodbye', 'Linus', 'Torvalds'),
+ ('goodbye', 'Richard', 'Stallman')
+ """
--- End diff --
still don't quite get how to use `counts`
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151185189
--- Diff: aria/parser/consumption/presentation.py ---
@@ -13,15 +13,16 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-
-from ...utils.threading import FixedThreadPoolExecutor
-from ...utils.formatting import json_dumps, yaml_dumps
+from ...utils.formatting import (json_dumps, yaml_dumps)
from ..loading import UriLocation
-from ..reading import AlreadyReadException
from ..presentation import PresenterNotFoundError
from .consumer import Consumer
+PRESENTATION_CACHE = {}
+CANONICAL_LOCATION_CACHE = {}
+
+
class Read(Consumer):
--- End diff --
The methods of these class should be documented, including explanations of the relationships between them.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151468906
--- Diff: extensions/aria_extension_tosca/simple_v1_0/assignments.py ---
@@ -144,6 +144,17 @@ def _get_type(self, context):
# In RelationshipAssignment
the_type = the_type[0] # This could be a RelationshipTemplate
+ if isinstance(self._container._container, RequirementAssignment):
--- End diff --
or maybe, if this kind of pattern repeats itself, consider creating a function that iteratively gets a nested attributes provided an iterable of attribute names.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153006931
--- Diff: aria/parser/consumption/presentation.py ---
@@ -31,47 +32,33 @@ class Read(Consumer):
instances.
It supports agnostic raw data composition for presenters that have
- ``_get_import_locations`` and ``_merge_import``.
+ ``_get_import_locations``, ``_validate_import``, and ``_merge_import``.
To improve performance, loaders are called asynchronously on separate threads.
Note that parsing may internally trigger more than one loading/reading/presentation
cycle, for example if the agnostic raw data has dependencies that must also be parsed.
"""
- def consume(self):
- if self.context.presentation.location is None:
- self.context.validation.report('Presentation consumer: missing location')
- return
-
- presenter = None
- imported_presentations = None
+ def __init__(self, context):
+ super(Read, self).__init__(context)
+ self._cache = {}
- executor = FixedThreadPoolExecutor(size=self.context.presentation.threads,
- timeout=self.context.presentation.timeout)
- executor.print_exceptions = self.context.presentation.print_exceptions
- try:
- presenter = self._present(self.context.presentation.location, None, None, executor)
- executor.drain()
-
- # Handle exceptions
- for e in executor.exceptions:
- self._handle_exception(e)
+ def consume(self):
+ # Present the main location and all imports recursively
+ main, results = self._present_all()
- imported_presentations = executor.returns
- finally:
- executor.close()
+ # Merge presentations
+ main.merge(results, self.context)
- # Merge imports
- if (imported_presentations is not None) and hasattr(presenter, '_merge_import'):
- for imported_presentation in imported_presentations:
- okay = True
- if hasattr(presenter, '_validate_import'):
- okay = presenter._validate_import(self.context, imported_presentation)
- if okay:
- presenter._merge_import(imported_presentation)
+ # Cache merged presentations
+ if self.context.presentation.cache:
+ for result in results:
+ result.cache()
--- End diff --
Yes, it needs to be global for high-efficiency concurrent testing: there, we have many threads each creating their own contexts and consumers, but we want them to all enjoy the cached results.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150777274
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
--- End diff --
why passing None? can't we set default values for `origin_canonical_location` and `origin_presenter_class`?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150785086
--- Diff: aria/parser/presentation/context.py ---
@@ -63,3 +67,12 @@ def get_from_dict(self, *names):
"""
return self.presenter._get_from_dict(*names) if self.presenter is not None else None
+
+ def create_executor(self):
+ if self.threads == 1:
--- End diff --
maybe we need to provide a way of configuring the threads through the initiator (and not necessarily overriding it in subclasses)?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153007549
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
--- End diff --
It's impossible to separate because they are produced together via the thread pool. I'll add method documentation to clarify.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013058
--- Diff: tests/mechanisms/utils.py ---
@@ -0,0 +1,71 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import itertools
+
+
+def matrix(*iterables, **kwargs):
+ """
+ Generates a matrix of parameters for ``@pytest.mark.parametrize``.
+
+ The matrix is essentially the Cartesian product of the arguments (which should be iterables),
+ with the added ability to "flatten" each value by breaking up tuples and recombining them into a
+ final flat value.
+
+ To do such recombination, use the ``counts`` argument (tuple) to specify the number of elements
+ per value in order. Any count greater than 1 (the default) enables recombination of that value.
+
+ Example::
+
+ x = ('hello', 'goodbye')
+ y = ('Linus', 'Richard')
+ matrix(x, y) ->
+ ('hello', 'Linus'),
+ ('hello', 'Richard'),
+ ('goodbye', 'Linus'),
+ ('goodbye', 'Richard')
+
+ y = (('Linus', 'Torvalds'), ('Richard', 'Stallman'))
+ matrix(x, y) ->
+ ('hello', ('Linus', 'Torvalds')),
+ ('hello', ('Richard', 'Stallman')),
+ ('goodbye', ('Linus', 'Torvalds')),
+ ('goodbye', ('Richard', 'Stallman'))
+
+ matrix(x, y, counts=(1, 2)) ->
+ ('hello', 'Linus', 'Torvalds'),
+ ('hello', 'Richard', 'Stallman'),
+ ('goodbye', 'Linus', 'Torvalds'),
+ ('goodbye', 'Richard', 'Stallman')
+ """
--- End diff --
I will add more documentation.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153006807
--- Diff: aria/parser/consumption/presentation.py ---
@@ -31,47 +32,33 @@ class Read(Consumer):
instances.
It supports agnostic raw data composition for presenters that have
- ``_get_import_locations`` and ``_merge_import``.
+ ``_get_import_locations``, ``_validate_import``, and ``_merge_import``.
To improve performance, loaders are called asynchronously on separate threads.
Note that parsing may internally trigger more than one loading/reading/presentation
cycle, for example if the agnostic raw data has dependencies that must also be parsed.
"""
- def consume(self):
- if self.context.presentation.location is None:
- self.context.validation.report('Presentation consumer: missing location')
- return
-
- presenter = None
- imported_presentations = None
+ def __init__(self, context):
+ super(Read, self).__init__(context)
+ self._cache = {}
- executor = FixedThreadPoolExecutor(size=self.context.presentation.threads,
- timeout=self.context.presentation.timeout)
- executor.print_exceptions = self.context.presentation.print_exceptions
- try:
- presenter = self._present(self.context.presentation.location, None, None, executor)
- executor.drain()
-
- # Handle exceptions
- for e in executor.exceptions:
- self._handle_exception(e)
+ def consume(self):
+ # Present the main location and all imports recursively
+ main, results = self._present_all()
--- End diff --
+1 renamed to "main_result" and "all_results"
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013856
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
--- End diff --
+1
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153008552
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
--- End diff --
I don't want to force parser writers to inherit from PresentationBase. They are free to create their own classes as long as they support these methods. Throughout the parser code you'll see these kinds of "hasattr" tests to see if the classes have these features.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153343795
--- Diff: aria/parser/reading/reader.py ---
@@ -28,16 +28,9 @@ def __init__(self, context, location, loader):
def load(self):
with OpenClose(self.loader) as loader:
--- End diff --
Because `__enter__` and `__exit__` are magic functions, and it seems very awkward to me to have users call them directly. There are not supposed to be called directly. However, in this case, I do think open and close should be part of the class's protocol.
As for using a mixin ... seems far more awkward to me than using a helper class. I still suggest leaving this as is.
There's no new code here, too -- it's what we had for a long time.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153011112
--- Diff: aria/utils/threading.py ---
@@ -93,7 +92,104 @@ def sum(arg1, arg2):
print executor.returns
"""
- _CYANIDE = object() # Special task marker used to kill worker threads.
+ def __init__(self, print_exceptions=False):
+ self.print_exceptions = print_exceptions
+
+ def submit(self, func, *args, **kwargs):
+ """
+ Submit a task for execution.
+
+ The task will be called ASAP on the next available worker thread in the pool.
+
+ :raises ExecutorException: if cannot be submitted
+ """
+ raise NotImplementedError
+
+ def close(self):
+ """
+ Blocks until all current tasks finish execution and all worker threads are dead.
+
+ You cannot submit tasks anymore after calling this.
+
+ This is called automatically upon exit if you are using the ``with`` keyword.
+ """
+ pass
+
+ def drain(self):
+ """
+ Blocks until all current tasks finish execution, but leaves the worker threads alive.
+ """
+ pass
+
+ @property
+ def returns(self):
+ """
+ The returned values from all tasks, in order of submission.
+ """
+ return ()
+
+ @property
+ def exceptions(self):
+ """
+ The raised exceptions from all tasks, in order of submission.
+ """
+ return ()
+
+ def raise_first(self):
+ """
+ If exceptions were thrown by any task, then the first one will be raised.
+
+ This is rather arbitrary: proper handling would involve iterating all the exceptions.
+ However, if you want to use the "raise" mechanism, you are limited to raising only one of
+ them.
+ """
+
+ exceptions = self.exceptions
+ if exceptions:
+ raise exceptions[0]
+
+ def __enter__(self):
+ return self
+
+ def __exit__(self, the_type, value, traceback):
+ pass
+
+
+class BlockingExecutor(Executor):
+ """
+ Executes tasks in the current thread.
+ """
+
+ def __init__(self, print_exceptions=False):
+ super(BlockingExecutor, self).__init__(print_exceptions=print_exceptions)
--- End diff --
Sorry guys, I disagree with both of you. I think each class should be explicit as possible in the arguments it accepts (so not kwargs) but also think we should reuse the logic of the base class (so send it to the super's `__init__`). I would rather keep this as is.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153008582
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
+ def __init__(self, presentation, canonical_location, origin_canonical_location):
+ self.presentation = presentation
+ self.canonical_location = canonical_location
+ self.origin_canonical_location = origin_canonical_location
+ self.merged = False
+
+ def get_imports(self, results):
+ imports = []
+
+ def has_import(result):
+ for i in imports:
+ if i.canonical_location == result.canonical_location:
+ return True
+ return False
+
+ for result in results:
+ if result.origin_canonical_location == self.canonical_location:
+ if not has_import(result):
+ imports.append(result)
+ return imports
+
+ def merge(self, results, context):
+ # Make sure to only merge each presentation once
+ if self.merged:
+ return
+ self.merged = True
+ for result in results:
+ if result.presentation == self.presentation:
+ result.merged = True
+
+ for result in self.get_imports(results):
+ # Make sure import is merged
+ result.merge(results, context)
+
+ # Validate import
+ if hasattr(self.presentation, '_validate_import'):
+ if not self.presentation._validate_import(context, result.presentation):
+ # _validate_import will report an issue if invalid
+ continue
+
+ # Merge import
+ if hasattr(self.presentation, '_merge_import'):
--- End diff --
As above.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153008314
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
--- End diff --
Perhaps the goal wasn't clear here: "location" is relative, while "canonical_location" is globally absolute. So the cache key has to be a combination of both, hence a tuple. I will add documentation to clarify this.
---
[GitHub] incubator-ariatosca issue #207: ARIA-1 Parser test suite
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/207
Can one of the admins verify this patch?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013051
--- Diff: tests/mechanisms/utils.py ---
@@ -0,0 +1,71 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import itertools
+
+
+def matrix(*iterables, **kwargs):
+ """
+ Generates a matrix of parameters for ``@pytest.mark.parametrize``.
--- End diff --
Because `*iterables` can be any length... if you use both `*` and `**` how else can specify a named parameter?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153011903
--- Diff: tests/extensions/aria_extension_tosca/conftest.py ---
@@ -0,0 +1,45 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""
+PyTest configuration module.
+
+Add support for a "--tosca-parser" CLI option.
+"""
+
+import pytest
+
+from ...mechanisms.parsing.aria import AriaParser
+
+
+def pytest_addoption(parser):
--- End diff --
These are PyTest-defined hooks. I will add a link to the PyTest documentation.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150776396
--- Diff: aria/parser/consumption/presentation.py ---
@@ -31,47 +32,33 @@ class Read(Consumer):
instances.
It supports agnostic raw data composition for presenters that have
- ``_get_import_locations`` and ``_merge_import``.
+ ``_get_import_locations``, ``_validate_import``, and ``_merge_import``.
To improve performance, loaders are called asynchronously on separate threads.
Note that parsing may internally trigger more than one loading/reading/presentation
cycle, for example if the agnostic raw data has dependencies that must also be parsed.
"""
- def consume(self):
- if self.context.presentation.location is None:
- self.context.validation.report('Presentation consumer: missing location')
- return
-
- presenter = None
- imported_presentations = None
+ def __init__(self, context):
+ super(Read, self).__init__(context)
+ self._cache = {}
- executor = FixedThreadPoolExecutor(size=self.context.presentation.threads,
- timeout=self.context.presentation.timeout)
- executor.print_exceptions = self.context.presentation.print_exceptions
- try:
- presenter = self._present(self.context.presentation.location, None, None, executor)
- executor.drain()
-
- # Handle exceptions
- for e in executor.exceptions:
- self._handle_exception(e)
+ def consume(self):
+ # Present the main location and all imports recursively
+ main, results = self._present_all()
- imported_presentations = executor.returns
- finally:
- executor.close()
+ # Merge presentations
+ main.merge(results, self.context)
- # Merge imports
- if (imported_presentations is not None) and hasattr(presenter, '_merge_import'):
- for imported_presentation in imported_presentations:
- okay = True
- if hasattr(presenter, '_validate_import'):
- okay = presenter._validate_import(self.context, imported_presentation)
- if okay:
- presenter._merge_import(imported_presentation)
+ # Cache merged presentations
+ if self.context.presentation.cache:
+ for result in results:
+ result.cache()
--- End diff --
does `PRESENTATION_CACHE` have to be global? sound more sensible it'd be a part of the `Read` consumer
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150775578
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
+ def __init__(self, presentation, canonical_location, origin_canonical_location):
+ self.presentation = presentation
+ self.canonical_location = canonical_location
+ self.origin_canonical_location = origin_canonical_location
+ self.merged = False
+
+ def get_imports(self, results):
+ imports = []
+
+ def has_import(result):
+ for i in imports:
+ if i.canonical_location == result.canonical_location:
+ return True
+ return False
+
+ for result in results:
+ if result.origin_canonical_location == self.canonical_location:
+ if not has_import(result):
+ imports.append(result)
+ return imports
+
+ def merge(self, results, context):
+ # Make sure to only merge each presentation once
+ if self.merged:
+ return
+ self.merged = True
+ for result in results:
+ if result.presentation == self.presentation:
+ result.merged = True
+
+ for result in self.get_imports(results):
+ # Make sure import is merged
+ result.merge(results, context)
+
+ # Validate import
+ if hasattr(self.presentation, '_validate_import'):
--- End diff --
maybe add `_validate_import` to `PresentationBase` to support default behavior. Checking for attribute and calling it might seem awkward
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151087783
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
--- End diff --
Should document the purpose of this class
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150820315
--- Diff: tests/extensions/aria_extension_tosca/simple_v1_0/data.py ---
@@ -0,0 +1,82 @@
+# -*- coding: utf-8 -*-
--- End diff --
why is it called data.py and not constants.py?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153014000
--- Diff: aria/parser/loading/uri.py ---
@@ -85,6 +91,11 @@ def close(self):
def load(self):
return self._loader.load() if self._loader is not None else None
+ def get_canonical_location(self):
+ self.open()
--- End diff --
For some loaders, yes.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153014351
--- Diff: aria/parser/presentation/presentation.py ---
@@ -199,6 +199,9 @@ class Presentation(PresentationBase):
"""
def _validate(self, context):
+ if (not context.presentation.configuration.get('validate_normative', True)) \
+ and self._get_extension('normative'):
+ return
--- End diff --
That user should never be using our ARIA `_extensions` and setting `normative: true`. These `_extensions` are entirely for internal use by our project. If they really want to use this feature, it is up to them to make sure the types are valid. (We test for this as part of our tox test suite by disabling this flag.)
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151408981
--- Diff: aria/utils/threading.py ---
@@ -93,7 +92,104 @@ def sum(arg1, arg2):
print executor.returns
"""
- _CYANIDE = object() # Special task marker used to kill worker threads.
+ def __init__(self, print_exceptions=False):
+ self.print_exceptions = print_exceptions
+
+ def submit(self, func, *args, **kwargs):
+ """
+ Submit a task for execution.
+
+ The task will be called ASAP on the next available worker thread in the pool.
+
+ :raises ExecutorException: if cannot be submitted
+ """
+ raise NotImplementedError
+
+ def close(self):
+ """
+ Blocks until all current tasks finish execution and all worker threads are dead.
+
+ You cannot submit tasks anymore after calling this.
+
+ This is called automatically upon exit if you are using the ``with`` keyword.
+ """
+ pass
+
+ def drain(self):
+ """
+ Blocks until all current tasks finish execution, but leaves the worker threads alive.
+ """
+ pass
+
+ @property
+ def returns(self):
+ """
+ The returned values from all tasks, in order of submission.
+ """
+ return ()
+
+ @property
+ def exceptions(self):
+ """
+ The raised exceptions from all tasks, in order of submission.
+ """
+ return ()
+
+ def raise_first(self):
+ """
+ If exceptions were thrown by any task, then the first one will be raised.
+
+ This is rather arbitrary: proper handling would involve iterating all the exceptions.
+ However, if you want to use the "raise" mechanism, you are limited to raising only one of
+ them.
+ """
+
+ exceptions = self.exceptions
+ if exceptions:
+ raise exceptions[0]
+
+ def __enter__(self):
+ return self
+
+ def __exit__(self, the_type, value, traceback):
+ pass
+
+
+class BlockingExecutor(Executor):
+ """
+ Executes tasks in the current thread.
+ """
+
+ def __init__(self, print_exceptions=False):
+ super(BlockingExecutor, self).__init__(print_exceptions=print_exceptions)
--- End diff --
In contrary to Max, since the `Executor` class already has `print_exceptions=False` in its `__init__`, why is it needed here? just call the super `__init__`, and then set attributes as you wish.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153016106
--- Diff: aria/modeling/service_common.py ---
@@ -587,12 +588,11 @@ class MetadataBase(TemplateModelMixin):
:ivar name: name
:vartype name: basestring
:ivar value: value
- :vartype value: basestring
"""
__tablename__ = 'metadata'
- value = Column(Text)
+ value = Column(PickleType)
--- End diff --
Why not? :)
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013980
--- Diff: aria/parser/loading/uri.py ---
@@ -44,6 +45,7 @@ def __init__(self, context, location, origin_location=None):
self.location = location
self._prefixes = StrictList(value_class=basestring)
self._loader = None
+ self._canonical_location = None
--- End diff --
I think "canonical" is a well-known adjective for file systems and URIs and might not need explanation. "Canonical" means globally absolute. If you think it should be documented, then where? It is used a lot.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153012216
--- Diff: tests/extensions/aria_extension_tosca/simple_v1_0/data.py ---
@@ -0,0 +1,82 @@
+# -*- coding: utf-8 -*-
--- End diff --
Because 1) Python doesn't have a "constants" feature, and 2) even if it did, that would be an implementation trait, and these data points would still work as data points if they weren't constants. I don't see why you would want to name the file for a technical aspect of its content. It would be like separating this file into `floats.py` and `ints.py` -- that doesn't help understanding usage.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151419226
--- Diff: aria/utils/threading.py ---
@@ -161,11 +242,7 @@ def close(self):
self._workers = None
def drain(self):
- """
- Blocks until all current tasks finish execution, but leaves the worker threads alive.
- """
-
- self._tasks.join() # oddly, the API does not support a timeout parameter
+ self._tasks.join() # oddly, the API does not support a timeout parameter
--- End diff --
In general, according to PEP8, `Inline comments should be separated by at least two spaces from the statement`.
https://www.python.org/dev/peps/pep-0008/#id32
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150787274
--- Diff: aria/utils/threading.py ---
@@ -93,7 +92,104 @@ def sum(arg1, arg2):
print executor.returns
"""
- _CYANIDE = object() # Special task marker used to kill worker threads.
+ def __init__(self, print_exceptions=False):
+ self.print_exceptions = print_exceptions
+
+ def submit(self, func, *args, **kwargs):
+ """
+ Submit a task for execution.
+
+ The task will be called ASAP on the next available worker thread in the pool.
+
+ :raises ExecutorException: if cannot be submitted
+ """
+ raise NotImplementedError
+
+ def close(self):
+ """
+ Blocks until all current tasks finish execution and all worker threads are dead.
+
+ You cannot submit tasks anymore after calling this.
+
+ This is called automatically upon exit if you are using the ``with`` keyword.
+ """
+ pass
+
+ def drain(self):
+ """
+ Blocks until all current tasks finish execution, but leaves the worker threads alive.
+ """
+ pass
+
+ @property
+ def returns(self):
+ """
+ The returned values from all tasks, in order of submission.
+ """
+ return ()
+
+ @property
+ def exceptions(self):
+ """
+ The raised exceptions from all tasks, in order of submission.
+ """
+ return ()
+
+ def raise_first(self):
+ """
+ If exceptions were thrown by any task, then the first one will be raised.
+
+ This is rather arbitrary: proper handling would involve iterating all the exceptions.
+ However, if you want to use the "raise" mechanism, you are limited to raising only one of
+ them.
+ """
+
+ exceptions = self.exceptions
+ if exceptions:
+ raise exceptions[0]
+
+ def __enter__(self):
+ return self
+
+ def __exit__(self, the_type, value, traceback):
+ pass
+
+
+class BlockingExecutor(Executor):
+ """
+ Executes tasks in the current thread.
+ """
+
+ def __init__(self, print_exceptions=False):
+ super(BlockingExecutor, self).__init__(print_exceptions=print_exceptions)
--- End diff --
if `print_exceptions` is just being passed to the parent constructor, consider using *args, **kwargs and passing those instead
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151402149
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
+ def __init__(self, presentation, canonical_location, origin_canonical_location):
+ self.presentation = presentation
+ self.canonical_location = canonical_location
+ self.origin_canonical_location = origin_canonical_location
+ self.merged = False
+
+ def get_imports(self, results):
+ imports = []
+
+ def has_import(result):
+ for i in imports:
+ if i.canonical_location == result.canonical_location:
+ return True
+ return False
+
+ for result in results:
+ if result.origin_canonical_location == self.canonical_location:
+ if not has_import(result):
+ imports.append(result)
+ return imports
+
+ def merge(self, results, context):
+ # Make sure to only merge each presentation once
+ if self.merged:
+ return
+ self.merged = True
+ for result in results:
+ if result.presentation == self.presentation:
+ result.merged = True
+
+ for result in self.get_imports(results):
+ # Make sure import is merged
+ result.merge(results, context)
+
+ # Validate import
+ if hasattr(self.presentation, '_validate_import'):
+ if not self.presentation._validate_import(context, result.presentation):
+ # _validate_import will report an issue if invalid
+ continue
+
+ # Merge import
+ if hasattr(self.presentation, '_merge_import'):
+ self.presentation._merge_import(result.presentation)
+
+ def cache(self):
+ if not self.merged:
+ raise RuntimeError(u'Only merged presentations can be cached: {0}'
+ .format(self.canonical_location))
+ PRESENTATION_CACHE[self.canonical_location] = self.presentation
+
+
+class _Skip(Exception):
--- End diff --
Funny thing, in some place you can see that this class was named `AlreadyReadException`...
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150821736
--- Diff: tests/extensions/aria_extension_tosca/simple_v1_0/templates/common/test_template_interface.py ---
@@ -0,0 +1,914 @@
+# -*- coding: utf-8 -*-
--- End diff --
I'm not sure providing those macros helps (apart from code reuse, it's really hard to understand and write new tests).
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153010864
--- Diff: aria/utils/collections.py ---
@@ -220,27 +225,30 @@ def __setitem__(self, key, value, **_):
return super(StrictDict, self).__setitem__(key, value)
-def merge(dict_a, dict_b, path=None, strict=False):
+def merge(dict_a, dict_b, copy=True, strict=False, path=None):
--- End diff --
+1
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150778395
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
--- End diff --
This seems like a very convoluted way of inserting a canonical location into the cache
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151394538
--- Diff: aria/parser/loading/uri.py ---
@@ -44,6 +45,7 @@ def __init__(self, context, location, origin_location=None):
self.location = location
self._prefixes = StrictList(value_class=basestring)
self._loader = None
+ self._canonical_location = None
--- End diff --
Somewhere it should be explained what is the difference between a location and a canonical location.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153014611
--- Diff: aria/parser/reading/yaml.py ---
@@ -82,7 +84,11 @@ def read(self):
# see issue here:
# https://bitbucket.org/ruamel/yaml/issues/61/roundtriploader-causes-exceptions-with
#yaml_loader = yaml.RoundTripLoader(data)
- yaml_loader = yaml.SafeLoader(data)
+ try:
+ # Faster C-based loader, might not be available on all platforms
+ yaml_loader = yaml.CSafeLoader(data)
+ except BaseException:
--- End diff --
I think I'm sure ... I want the failover to always work. Even if CSafeLoader does exist but fails somehow internally, we should still make an effort to run.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153014418
--- Diff: aria/parser/reading/reader.py ---
@@ -28,16 +28,9 @@ def __init__(self, context, location, loader):
def load(self):
with OpenClose(self.loader) as loader:
--- End diff --
If you can refactor this to make it use context manager and still be clear, please show me.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150777009
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
--- End diff --
why returns both `main` and `results`? consider splitting into 2 different methods
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150817394
--- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
@@ -640,24 +649,33 @@ def create_node_filter_constraints(context, node_filter, target_node_template_co
for property_name, constraint_clause in properties:
constraint = create_constraint(context, node_filter, constraint_clause,
property_name, capability_name)
- target_node_template_constraints.append(constraint)
+ if constraint is not None:
+ target_node_template_constraints.append(constraint)
-def create_constraint(context, node_filter, constraint_clause, property_name, capability_name): # pylint: disable=too-many-return-statements
+def create_constraint(context, node_filter, constraint_clause, property_name, capability_name): # pylint: disable=too-many-return-statements
+ if (not isinstance(constraint_clause._raw, dict)) or (len(constraint_clause._raw) != 1):
+ context.validation.report(
+ u'node_filter constraint is not a dict with one key: {0}'
+ .format(safe_repr(constraint_clause._raw)),
+ locator=node_filter._locator,
+ level=Issue.FIELD)
+ return None
+
constraint_key = constraint_clause._raw.keys()[0]
the_type = constraint_clause._get_type(context)
- def coerce_constraint(constraint):
+ def coerce_constraint(constraint, the_type=the_type):
--- End diff --
consider renaming to `entity_type` or something similar.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150820649
--- Diff: tests/extensions/aria_extension_tosca/simple_v1_0/functions/test_function_concat.py ---
@@ -0,0 +1,102 @@
+ # -*- coding: utf-8 -*-
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+def test_functions_concat_syntax_empty(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [] }
+""").assert_success()
+
+
+def test_functions_concat_strings(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [ a, b, c ] }
+""").assert_success()
+
+
+def test_functions_concat_mixed(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [ a, 1, 1.1, null, [], {} ] }
+""").assert_success()
+
+
+def test_functions_concat_nested(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ MyType:
+ properties:
+ my_parameter:
+ type: string
+topology_template:
+ node_templates:
+ my_node:
+ type: MyType
+ properties:
+ my_parameter: { concat: [ a, { concat: [ b, c ] } ] }
+""").assert_success()
+
+
+# Unicode
+
+def test_functions_concat_unicode(parser):
+ parser.parse_literal("""
+tosca_definitions_version: tosca_simple_yaml_1_0
+node_types:
+ 類型:
+ properties:
+ 參數:
+ type: string
+topology_template:
+ node_templates:
+ 模板:
+ type: 類型
+ properties:
+ 參數: { concat: [ 一, 二, 三 ] }
+""").assert_success()
--- End diff --
that's really weird we are testing it passes but don't check that the value is indeed what we expect. I do think that at the very least we should check the expected value was set
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150820028
--- Diff: tests/extensions/aria_extension_tosca/conftest.py ---
@@ -0,0 +1,45 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""
+PyTest configuration module.
+
+Add support for a "--tosca-parser" CLI option.
+"""
+
+import pytest
+
+from ...mechanisms.parsing.aria import AriaParser
+
+
+def pytest_addoption(parser):
--- End diff --
document functions
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150775602
--- Diff: aria/parser/consumption/presentation.py ---
@@ -86,52 +73,193 @@ def dump(self):
self.context.presentation.presenter._dump(self.context)
def _handle_exception(self, e):
- if isinstance(e, AlreadyReadException):
+ if isinstance(e, _Skip):
return
super(Read, self)._handle_exception(e)
- def _present(self, location, origin_location, presenter_class, executor):
+ def _present_all(self):
+ location = self.context.presentation.location
+
+ if location is None:
+ self.context.validation.report('Read consumer: missing location')
+ return
+
+ executor = self.context.presentation.create_executor()
+ try:
+ # This call may recursively submit tasks to the executor if there are imports
+ main = self._present(location, None, None, executor)
+
+ # Wait for all tasks to complete
+ executor.drain()
+
+ # Handle exceptions
+ for e in executor.exceptions:
+ self._handle_exception(e)
+
+ results = executor.returns or []
+ finally:
+ executor.close()
+
+ results.insert(0, main)
+
+ return main, results
+
+ def _present(self, location, origin_canonical_location, origin_presenter_class, executor):
# Link the context to this thread
self.context.set_thread_local()
- raw = self._read(location, origin_location)
+ # Canonicalize the location
+ if self.context.reading.reader is None:
+ loader, canonical_location = self._create_loader(location, origin_canonical_location)
+ else:
+ # If a reader is specified in the context then we skip loading
+ loader = None
+ canonical_location = location
+
+ # Skip self imports
+ if canonical_location == origin_canonical_location:
+ raise _Skip()
+
+ if self.context.presentation.cache:
+ # Is the presentation in the global cache?
+ try:
+ presentation = PRESENTATION_CACHE[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ try:
+ # Is the presentation in the local cache?
+ presentation = self._cache[canonical_location]
+ return _Result(presentation, canonical_location, origin_canonical_location)
+ except KeyError:
+ pass
+
+ # Create and cache new presentation
+ presentation = self._create_presentation(canonical_location, loader, origin_presenter_class)
+ self._cache[canonical_location] = presentation
+ # Submit imports to executor
+ if hasattr(presentation, '_get_import_locations'):
+ import_locations = presentation._get_import_locations(self.context)
+ if import_locations:
+ for import_location in import_locations:
+ import_location = UriLocation(import_location)
+ executor.submit(self._present, import_location, canonical_location,
+ presentation.__class__, executor)
+
+ return _Result(presentation, canonical_location, origin_canonical_location)
+
+ def _create_loader(self, location, origin_canonical_location):
+ loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
+ origin_canonical_location)
+
+ canonical_location = None
+
+ if origin_canonical_location is not None:
+ cache_key = (origin_canonical_location, location)
+ try:
+ canonical_location = CANONICAL_LOCATION_CACHE[cache_key]
+ return loader, canonical_location
+ except KeyError:
+ pass
+ else:
+ cache_key = None
+
+ canonical_location = loader.get_canonical_location()
+
+ # Because retrieving the canonical location can be costly, we will try to cache it
+ if cache_key is not None:
+ CANONICAL_LOCATION_CACHE[cache_key] = canonical_location
+
+ return loader, canonical_location
+
+ def _create_presentation(self, canonical_location, loader, default_presenter_class):
+ # The reader we specified in the context will override
+ reader = self.context.reading.reader
+
+ if reader is None:
+ # Read raw data from loader
+ reader = self.context.reading.reader_source.get_reader(self.context.reading,
+ canonical_location, loader)
+
+ raw = reader.read()
+
+ # Wrap raw data in presenter class
if self.context.presentation.presenter_class is not None:
- # The presenter class we specified in the context overrides everything
+ # The presenter class we specified in the context will override
presenter_class = self.context.presentation.presenter_class
else:
try:
presenter_class = self.context.presentation.presenter_source.get_presenter(raw)
except PresenterNotFoundError:
- if presenter_class is None:
+ if default_presenter_class is None:
raise
- # We'll use the presenter class we were given (from the presenter that imported us)
- if presenter_class is None:
- raise PresenterNotFoundError('presenter not found')
+ else:
+ presenter_class = default_presenter_class
+
+ if presenter_class is None:
+ raise PresenterNotFoundError(u'presenter not found: {0}'.format(canonical_location))
presentation = presenter_class(raw=raw)
- if presentation is not None and hasattr(presentation, '_link_locators'):
+ if hasattr(presentation, '_link_locators'):
presentation._link_locators()
- # Submit imports to executor
- if hasattr(presentation, '_get_import_locations'):
- import_locations = presentation._get_import_locations(self.context)
- if import_locations:
- for import_location in import_locations:
- # The imports inherit the parent presenter class and use the current location as
- # their origin location
- import_location = UriLocation(import_location)
- executor.submit(self._present, import_location, location, presenter_class,
- executor)
-
return presentation
- def _read(self, location, origin_location):
- if self.context.reading.reader is not None:
- return self.context.reading.reader.read()
- loader = self.context.loading.loader_source.get_loader(self.context.loading, location,
- origin_location)
- reader = self.context.reading.reader_source.get_reader(self.context.reading, location,
- loader)
- return reader.read()
+
+class _Result(object):
+ def __init__(self, presentation, canonical_location, origin_canonical_location):
+ self.presentation = presentation
+ self.canonical_location = canonical_location
+ self.origin_canonical_location = origin_canonical_location
+ self.merged = False
+
+ def get_imports(self, results):
+ imports = []
+
+ def has_import(result):
+ for i in imports:
+ if i.canonical_location == result.canonical_location:
+ return True
+ return False
+
+ for result in results:
+ if result.origin_canonical_location == self.canonical_location:
+ if not has_import(result):
+ imports.append(result)
+ return imports
+
+ def merge(self, results, context):
+ # Make sure to only merge each presentation once
+ if self.merged:
+ return
+ self.merged = True
+ for result in results:
+ if result.presentation == self.presentation:
+ result.merged = True
+
+ for result in self.get_imports(results):
+ # Make sure import is merged
+ result.merge(results, context)
+
+ # Validate import
+ if hasattr(self.presentation, '_validate_import'):
+ if not self.presentation._validate_import(context, result.presentation):
+ # _validate_import will report an issue if invalid
+ continue
+
+ # Merge import
+ if hasattr(self.presentation, '_merge_import'):
--- End diff --
ditto
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153009548
--- Diff: aria/parser/reading/yaml.py ---
@@ -16,18 +16,30 @@
from .reader import Reader
from .locator import Locator
from .exceptions import ReaderSyntaxError
-from .locator import LocatableString, LocatableInt, LocatableFloat
+from .locator import (LocatableString, LocatableInt, LocatableFloat)
-# Add our types to ruamel.yaml
+
+MERGE_TAG = u'tag:yaml.org,2002:merge'
+MAP_TAG = u'tag:yaml.org,2002:map'
+
+
+# Add our types to RoundTripRepresenter
yaml.representer.RoundTripRepresenter.add_representer(
LocatableString, yaml.representer.RoundTripRepresenter.represent_unicode)
yaml.representer.RoundTripRepresenter.add_representer(
LocatableInt, yaml.representer.RoundTripRepresenter.represent_int)
yaml.representer.RoundTripRepresenter.add_representer(
LocatableFloat, yaml.representer.RoundTripRepresenter.represent_float)
-MERGE_TAG = u'tag:yaml.org,2002:merge'
-MAP_TAG = u'tag:yaml.org,2002:map'
+
+def construct_yaml_map(self, node):
--- End diff --
+1
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150787953
--- Diff: examples/hello-world/hello-world.yaml ---
@@ -2,30 +2,24 @@ tosca_definitions_version: tosca_simple_yaml_1_0
node_types:
- WebServer:
- derived_from: tosca:Root
- capabilities:
- host:
- type: tosca:Container
-
- WebApp:
+ HelloWorld:
--- End diff --
I think that the example documentation might needed an update as well :smiley:
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r149664387
--- Diff: aria/modeling/service_common.py ---
@@ -587,12 +588,11 @@ class MetadataBase(TemplateModelMixin):
:ivar name: name
:vartype name: basestring
:ivar value: value
- :vartype value: basestring
"""
__tablename__ = 'metadata'
- value = Column(Text)
+ value = Column(PickleType)
--- End diff --
how can a value be of pickletype?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151184355
--- Diff: aria/parser/consumption/presentation.py ---
@@ -31,47 +32,33 @@ class Read(Consumer):
instances.
It supports agnostic raw data composition for presenters that have
- ``_get_import_locations`` and ``_merge_import``.
+ ``_get_import_locations``, ``_validate_import``, and ``_merge_import``.
To improve performance, loaders are called asynchronously on separate threads.
Note that parsing may internally trigger more than one loading/reading/presentation
cycle, for example if the agnostic raw data has dependencies that must also be parsed.
"""
- def consume(self):
- if self.context.presentation.location is None:
- self.context.validation.report('Presentation consumer: missing location')
- return
-
- presenter = None
- imported_presentations = None
+ def __init__(self, context):
+ super(Read, self).__init__(context)
+ self._cache = {}
- executor = FixedThreadPoolExecutor(size=self.context.presentation.threads,
- timeout=self.context.presentation.timeout)
- executor.print_exceptions = self.context.presentation.print_exceptions
- try:
- presenter = self._present(self.context.presentation.location, None, None, executor)
- executor.drain()
-
- # Handle exceptions
- for e in executor.exceptions:
- self._handle_exception(e)
+ def consume(self):
+ # Present the main location and all imports recursively
+ main, results = self._present_all()
--- End diff --
since `main` and each result from `results` are both of type `_Result`, these names are confusing.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153011276
--- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
@@ -640,24 +649,33 @@ def create_node_filter_constraints(context, node_filter, target_node_template_co
for property_name, constraint_clause in properties:
constraint = create_constraint(context, node_filter, constraint_clause,
property_name, capability_name)
- target_node_template_constraints.append(constraint)
+ if constraint is not None:
+ target_node_template_constraints.append(constraint)
-def create_constraint(context, node_filter, constraint_clause, property_name, capability_name): # pylint: disable=too-many-return-statements
+def create_constraint(context, node_filter, constraint_clause, property_name, capability_name): # pylint: disable=too-many-return-statements
+ if (not isinstance(constraint_clause._raw, dict)) or (len(constraint_clause._raw) != 1):
+ context.validation.report(
+ u'node_filter constraint is not a dict with one key: {0}'
+ .format(safe_repr(constraint_clause._raw)),
+ locator=node_filter._locator,
+ level=Issue.FIELD)
+ return None
+
constraint_key = constraint_clause._raw.keys()[0]
the_type = constraint_clause._get_type(context)
- def coerce_constraint(constraint):
+ def coerce_constraint(constraint, the_type=the_type):
--- End diff --
+1 renamed to `value_type`
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153011505
--- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/data_types.py ---
@@ -318,15 +318,15 @@ def report(message, constraint):
#
def get_data_type_value(context, presentation, field_name, type_name):
- the_type = get_type_by_name(context, type_name, 'data_types')
- if the_type is not None:
- value = getattr(presentation, field_name)
- if value is not None:
+ value = getattr(presentation, field_name)
+ if value is not None:
+ the_type = get_type_by_name(context, type_name, 'data_types')
--- End diff --
+1 to `data_type`
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153152535
--- Diff: aria/parser/loading/uri.py ---
@@ -44,6 +45,7 @@ def __init__(self, context, location, origin_location=None):
self.location = location
self._prefixes = StrictList(value_class=basestring)
self._loader = None
+ self._canonical_location = None
--- End diff --
This is the only class that has both the `location` and the `_canonical_location` attributes. So just explain in this class what is the purpose of each one.
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r151396216
--- Diff: aria/parser/loading/uri.py ---
@@ -85,6 +91,11 @@ def close(self):
def load(self):
return self._loader.load() if self._loader is not None else None
+ def get_canonical_location(self):
+ self.open()
--- End diff --
Do we have to open and close in order for the canonical location to get updated?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153013725
--- Diff: aria/parser/consumption/presentation.py ---
@@ -31,47 +32,33 @@ class Read(Consumer):
instances.
It supports agnostic raw data composition for presenters that have
- ``_get_import_locations`` and ``_merge_import``.
+ ``_get_import_locations``, ``_validate_import``, and ``_merge_import``.
To improve performance, loaders are called asynchronously on separate threads.
Note that parsing may internally trigger more than one loading/reading/presentation
cycle, for example if the agnostic raw data has dependencies that must also be parsed.
"""
- def consume(self):
- if self.context.presentation.location is None:
- self.context.validation.report('Presentation consumer: missing location')
- return
-
- presenter = None
- imported_presentations = None
+ def __init__(self, context):
+ super(Read, self).__init__(context)
+ self._cache = {}
- executor = FixedThreadPoolExecutor(size=self.context.presentation.threads,
- timeout=self.context.presentation.timeout)
- executor.print_exceptions = self.context.presentation.print_exceptions
- try:
- presenter = self._present(self.context.presentation.location, None, None, executor)
- executor.drain()
-
- # Handle exceptions
- for e in executor.exceptions:
- self._handle_exception(e)
+ def consume(self):
+ # Present the main location and all imports recursively
+ main, results = self._present_all()
- imported_presentations = executor.returns
- finally:
- executor.close()
+ # Merge presentations
+ main.merge(results, self.context)
- # Merge imports
- if (imported_presentations is not None) and hasattr(presenter, '_merge_import'):
- for imported_presentation in imported_presentations:
- okay = True
- if hasattr(presenter, '_validate_import'):
- okay = presenter._validate_import(self.context, imported_presentation)
- if okay:
- presenter._merge_import(imported_presentation)
+ # Cache merged presentations
+ if self.context.presentation.cache:
+ for result in results:
+ result.cache()
- self.context.presentation.presenter = presenter
+ self.context.presentation.presenter = main.presentation
+ if main.canonical_location is not None:
--- End diff --
The loader might fail for some reason to turn the location into a canonical location
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r150785288
--- Diff: aria/parser/presentation/field_validators.py ---
@@ -14,12 +14,29 @@
# limitations under the License.
+from ...utils.formatting import safe_repr
from ..validation import Issue
from .utils import (parse_types_dict_names, report_issue_for_unknown_type,
report_issue_for_parent_is_self, report_issue_for_unknown_parent_type,
report_issue_for_circular_type_hierarchy)
+def not_negative_validator(field, presentation, context):
+ """
+ Makes sure that the field is not negative.
+
+ Can be used with the :func:`field_validator` decorator.
+ """
+
+ field.default_validate(presentation, context)
+ value = getattr(presentation, field.name)
+ if (value is not None) and (value < 0):
--- End diff --
can't the value be not `None` nor and number?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153195877
--- Diff: aria/parser/reading/reader.py ---
@@ -28,16 +28,9 @@ def __init__(self, context, location, loader):
def load(self):
with OpenClose(self.loader) as loader:
--- End diff --
can't you just add the `__enter__` and `__exit__` methods to the `Loader` class?
And if you want several classes to use this logic, just make a mixin class and inherit from it?
---
[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite
Posted by aviyoop <gi...@git.apache.org>.
Github user aviyoop commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/207#discussion_r153196429
--- Diff: aria/utils/threading.py ---
@@ -161,11 +242,7 @@ def close(self):
self._workers = None
def drain(self):
- """
- Blocks until all current tasks finish execution, but leaves the worker threads alive.
- """
-
- self._tasks.join() # oddly, the API does not support a timeout parameter
+ self._tasks.join() # oddly, the API does not support a timeout parameter
--- End diff --
OK. But to be exact, the 'mix of styles' is your style (and PRs you reviewed), and everyone else's style =)
---