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

[16.07] Do not allow Conda to replace Galaxy's Python environment for select tools. #3351

Merged

Conversation

jmchilton
Copy link
Member

This has never happened in my testing, but there have been multiple reports of Conda crafting environments for the set metadata tool that include Python. When this happens Galaxy's Python environment is lost and the tool cannot function properly.

For these tools, simply add the Conda's bin directory to the jobs PATH - certainly a hack but it covers samtools and any other use case I can readily conceive of. Datatypes or tools that require new Python libraries beside Galaxy's own - need to be added to our requirements.txt - that is how virtualenvs should work.

Fixes #3238.
Fixes #3224.
Fixes https://biostar.usegalaxy.org/p/20865/.

Ping @bgruening @mvdbeek.

@mvdbeek
Copy link
Member

mvdbeek commented Dec 21, 2016

This is working fine for me and solves a a problem that comes up frequently. 👍

self.environment_path
)
if self._preserve_python_environment:
return """export PATH=$PATH:'%s/bin' """ % (
Copy link
Member

Choose a reason for hiding this comment

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

Is PATH enough? I think conda activate modifies more env variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know that it is broken in some cases (e.g. Python modules) - but it works for samtools. I think PATH is good enough - but I'm open to doing more if you have specific ideas.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, we can extend this if the need emerges. Maybe a comment in the code would be good anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased with a comment as suggested - thanks for the advice I agree completely.

This has never happened in my testing, but there have been multiple reports of Conda crafting environments for the set metadata tool that include Python. When this happens Galaxy's Python environment is lost and the tool cannot function properly.

Fixes galaxyproject#3238.
Fixes galaxyproject#3224.
Fixes https://biostar.usegalaxy.org/p/20865/.

Rebased to add a comment as suggested by @nsoranzo.
@jmchilton jmchilton force-pushed the galaxy_environment_fixes branch from 329ca24 to fe2a1d8 Compare December 21, 2016 18:33
@nsoranzo nsoranzo added this to the 16.07 milestone Dec 21, 2016
jmchilton added a commit to jmchilton/galaxy that referenced this pull request Dec 21, 2016
Tricky merge - please review.

Conflicts:
	lib/galaxy/tools/deps/resolvers/conda.py
jmchilton added a commit to jmchilton/galaxy that referenced this pull request Dec 21, 2016
@nsoranzo
Copy link
Member

API test failure seems unrelated, going to merge if no one screams in the next 10'.

@nsoranzo nsoranzo merged commit 8b75305 into galaxyproject:release_16.07 Dec 21, 2016
@bgruening
Copy link
Member

Sorry for being late to this game. I could at least figure out why this happens to many users. These users are new and setting up a fresh install - in this you don't have a samtools TS version in your tool_deps dir, so the package resolver is not finding it and falls back to conda, which has the env problem.

@jmchilton
Copy link
Member Author

@bgruening No - I wiped out my tool dependencies directory and this still happened. You can see the difference above where sometimes setuptools is installed with samtools via conda and sometimes it is not. It isn't like in my case samtools isn't being installed via Conda.

@bgruening
Copy link
Member

Not sure I understand, I mean it happens with conda, and there are now users that setting up Galaxy from scratch without a tool_dependencies dir and seeing this error because conda tries to resolve samtools. This explains why we are seeing this now more often.

Glad this is fixed! Thanks to all!

bgruening added a commit that referenced this pull request Dec 21, 2016
@jmchilton
Copy link
Member Author

jmchilton commented Dec 22, 2016

it happens with conda

Sometimes... @mvdbeek and I don't understand why sometimes it happens with Conda and sometimes it doesn't. Why is it when he and I install samtools we don't get any Python stuff in that environment and when others do - they do get Python stuff. I guess it is not important why with this fix in place - it is just confusing.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Feb 17, 2017
This is a less broken alternative to galaxyproject#3620 and something I intended to ultimately do anyway.

sam-to-bam used to be broken but now it isn't, we need to express that. Ideally we would move most of the things from the unversioned list to the versioned list as they are fixed.

xref galaxyproject#3620 (comment)
xref galaxyproject@fe2a1d8#commitcomment-20900607
xref galaxyproject#3351
xref galaxyproject/tools-iuc#1148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants