You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ariatosca.apache.org by mxmrlv <gi...@git.apache.org> on 2017/10/23 14:16:17 UTC
[GitHub] incubator-ariatosca pull request #199: ARIA-392 Failing to load ruamel.yaml
GitHub user mxmrlv opened a pull request:
https://github.com/apache/incubator-ariatosca/pull/199
ARIA-392 Failing to load ruamel.yaml
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-ariatosca ARIA-392-Failing-to-load-ruamel.yaml
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-ariatosca/pull/199.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 #199
----
commit 8e18062339446512411789b7f5129ddcf2331816
Author: max-orlov <ma...@gigaspaces.com>
Date: 2017-10-23T12:58:14Z
ARIA-392 Failing to load ruamel.yaml
----
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
so, i think we're ready
---
[GitHub] incubator-ariatosca pull request #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/199#discussion_r148628206
--- Diff: aria/__init__.py ---
@@ -17,14 +17,45 @@
The ARIA root package provides entry points for extension and storage initialization.
"""
+import os
import sys
+import types
from pkgutil import iter_modules
import pkg_resources
-
aria_package_name = 'apache-ariatosca'
__version__ = pkg_resources.get_distribution(aria_package_name).version
+
+
+
+try:
+ import ruamel # noqa: F401
+except ImportError:
--- End diff --
Another follow up: if this is still an issue with newer versions of ruaeml, I strongly suggest we open a bug at that project. I've done it in the past and have gotten things fixed. If you do that, make sure to link that bug report in a code comment here -- in case it's solved in the future, we should remove our workaround.
---
[GitHub] incubator-ariatosca pull request #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/199#discussion_r148620801
--- Diff: aria/__init__.py ---
@@ -17,14 +17,45 @@
The ARIA root package provides entry points for extension and storage initialization.
"""
+import os
import sys
+import types
from pkgutil import iter_modules
import pkg_resources
-
aria_package_name = 'apache-ariatosca'
__version__ = pkg_resources.get_distribution(aria_package_name).version
+
+
+
+try:
+ import ruamel # noqa: F401
--- End diff --
I think we've decided to remove IDE-specific comments.
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
Still, I don't see why it should be in `aria/__init__.py` and be always
activated. I think it should be in activated only for a module that
specifically is using ruamel. It would still only happen once.
On Wed, Nov 8, 2017 at 2:31 PM, Maxim Orlov <no...@github.com>
wrote:
> This code only runs once, it doesn't provide with a patched up ruamel, it
> patched the loading mechanism...so loading should work everywhere, as long
> as the code was run...
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-ariatosca/pull/199#issuecomment-342949895>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABF_4dyDXJXUA4TT4Mc2ofbzAiholfI7ks5s0g-7gaJpZM4QC6qG>
> .
>
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
OK. So I guess you don't want to change the imports in `/tests/`?
On Wed, Nov 8, 2017 at 4:32 PM, Maxim Orlov <no...@github.com>
wrote:
> so, i think we're ready
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-ariatosca/pull/199#issuecomment-342983490>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABF_4Z5-j-tU5KsVd0FKsJcbugbiCbdXks5s0iwOgaJpZM4QC6qG>
> .
>
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
@mxmrlv You didn't address these:
1. Let's move this to `from aria.utils.yaml import yaml`. I don't think the aria `__init__.py` is the right place for this.
2. Let's go over *all* places in the codebase where we import ruamel to use the above.
---
[GitHub] incubator-ariatosca pull request #199: ARIA-392 Failing to load ruamel.yaml
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/199#discussion_r148952959
--- Diff: aria/__init__.py ---
@@ -17,14 +17,45 @@
The ARIA root package provides entry points for extension and storage initialization.
"""
+import os
import sys
+import types
from pkgutil import iter_modules
import pkg_resources
-
aria_package_name = 'apache-ariatosca'
__version__ = pkg_resources.get_distribution(aria_package_name).version
+
+
+
+try:
+ import ruamel # noqa: F401
+except ImportError:
--- End diff --
The problem with ruamel is that it's missing the top level __init__. I have seen that the author of ruamel had no intention of fixing it. instead he supplied .pth files as part of the installation. This files do work in most cases but not when the destination dir is a custom dir. Placing this code under the `__init__.py` enables loading ruamel from each submodule of ARIA. I'm not sure how placing it under `aria.utils.yaml` would be a better fix. But if you feel it's better we could do that.
---
[GitHub] incubator-ariatosca pull request #199: ARIA-392 Failing to load ruamel.yaml
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-ariatosca/pull/199
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
OK, np just wanted to make sure we're on the same page...
---
[GitHub] incubator-ariatosca pull request #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/199#discussion_r148621970
--- Diff: aria/__init__.py ---
@@ -17,14 +17,45 @@
The ARIA root package provides entry points for extension and storage initialization.
"""
+import os
import sys
+import types
from pkgutil import iter_modules
import pkg_resources
-
aria_package_name = 'apache-ariatosca'
__version__ = pkg_resources.get_distribution(aria_package_name).version
+
+
+
+try:
+ import ruamel # noqa: F401
+except ImportError:
--- End diff --
I recent commit removed Py2.6 support and allowed us to upgrade ruamel to its latest version. So, I suggest rebasing on master and testing again: perhaps this hack isn't needed anymore.
Assuming this hack is still needed, we need to find a way to generalize it, since we import ruamel in many, many places in the code. Perhaps we should have an {{aria.utils.yaml}} package where we do all this work once. Everywhere else we use yaml would then have to be replaced to {{from aria.utils.yaml import yaml}}. A nice side benefit would be that it would be easy to replace ruamel with a different yaml parser is that is ever necessary.
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
I think we are. Also: I only suggest, you of course can decide differently.
Maybe do one more commit to make sure we are on the same page? Also feel
free to rebase and squash, it's actually easier for a small PR like this.
On Wed, Nov 8, 2017 at 2:44 PM, Maxim Orlov <no...@github.com>
wrote:
> OK, np just wanted to make sure we're on the same page...
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-ariatosca/pull/199#issuecomment-342953301>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABF_4eMjoGH5Nm8Cy4NEX7TY_wpSXRR6ks5s0hKdgaJpZM4QC6qG>
> .
>
---
[GitHub] incubator-ariatosca pull request #199: ARIA-392 Failing to load ruamel.yaml
Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:
https://github.com/apache/incubator-ariatosca/pull/199#discussion_r149136650
--- Diff: aria/__init__.py ---
@@ -17,14 +17,45 @@
The ARIA root package provides entry points for extension and storage initialization.
"""
+import os
import sys
+import types
from pkgutil import iter_modules
import pkg_resources
-
aria_package_name = 'apache-ariatosca'
__version__ = pkg_resources.get_distribution(aria_package_name).version
+
+
+
+try:
+ import ruamel # noqa: F401
+except ImportError:
--- End diff --
I just think `from aria.utils.yaml import yaml` will give us a clean separation in case we have a better fix in the future, so I do prefer that.
Also, for this PR to merge we need to change *all* our uses of ruamel in the codebase to that import.
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
Can one of the admins verify this patch?
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
This code only runs once, it doesn't provide with a patched up ruamel, it patched the loading mechanism...so loading should work everywhere, as long as the code was run...
---
[GitHub] incubator-ariatosca issue #199: ARIA-392 Failing to load ruamel.yaml
Posted by mxmrlv <gi...@git.apache.org>.
Github user mxmrlv commented on the issue:
https://github.com/apache/incubator-ariatosca/pull/199
@tliron any additional comments? or should i merge?
---