Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-25431: Include conda environment in package version detection #45

Merged
merged 5 commits into from Aug 28, 2020

Conversation

timj
Copy link
Member

@timj timj commented Aug 27, 2020

No description provided.

@timj timj requested a review from brianv0 August 27, 2020 23:48
Copy link
Contributor

@brianv0 brianv0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems good enough - I would posit the 2 seconds in conda info would take much longer over NFS, especially in some circumstances, so I think this is fine.

@@ -278,6 +327,7 @@ def fromSystem(cls):
packages : `Packages`
"""
packages = {}
packages.update(getCondaPackages())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mentioned a slowdown, does fromSystem need to be cached in some way? Or is generally processed once per process?

Copy link
Member Author

@timj timj Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only expect this to be called once but on the other hand we can be pretty confident that it's not going to change whilst this process is running. It might be best to add some lru_cache around each of the get functions just in case it is called multiple times. @ktlim can you see a downside for doing that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altough there is a special case of the routine that gets the python packages since that only reports packages that have been loaded so if you call it, then do an import, then call it again, you will get a different answer. That won't be a problem for the Conda stuff or, I imagine, the EUPS one.

# info_json = run_command(Commands.INFO, "--json")
# As a comporomise look for the env name in the path to the python
# executable
match = re.search(r"/envs/(.*?)/bin/", sys.executable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to see if there was a slightly more elegant way of doing this without much luck. This may be about as good as it gets, unfortunately. Conda does seem to know some information about the environment(s) was activated, but I wasn't able to find a much better way than this.

@timj
Copy link
Member Author

timj commented Aug 28, 2020

@brianv0 I added a cache for the conda and eups results.

@timj timj merged commit 357b56b into master Aug 28, 2020
@timj timj deleted the tickets/DM-25431 branch August 28, 2020 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants