[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[tliron]

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

[tliron]

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

[tliron]

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

[aviyoop]

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 =)


---

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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 pull request #207: ARIA-1 Parser test suite

[aviyoop]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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

[tliron]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[tliron]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[tliron]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[tliron]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[tliron]

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 pull request #207: ARIA-1 Parser test suite

[tliron]

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

[tliron]

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

[aviyoop]

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):
+  

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[aviyoop]

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):
+  

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[aviyoop]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

Github user mxmrlv commented on a diff in the pull request:

https://github.com/apache/incubator-ariatosca/pull/207#discussion_r15091
  
--- 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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[mxmrlv]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[mxmrlv]

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

[mxmrlv]

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):
+   

[GitHub] incubator-ariatosca pull request #207: ARIA-1 Parser test suite

[mxmrlv]

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

[tliron]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[mxmrlv]

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

[tliron]

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 
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.




---