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

Limit adjusting Galaxy's Python environment to legacy tools. #3364

Merged
merged 1 commit into from Jan 3, 2017

Conversation

Projects
None yet
6 participants
@jmchilton
Copy link
Member

jmchilton commented Dec 23, 2016

Don't expose Galaxy's environment to tools unless there is evidence we need to. This should restrict future bugs by limitting what tools are exposed to Galaxy's internals and will hopefully fix bugs related to Conda dependencies masking Galaxy - such as #2994.

Fixes #2994.

@galaxybot galaxybot added this to the 17.01 milestone Dec 23, 2016

@jmchilton jmchilton force-pushed the jmchilton:clean_python branch 3 times, most recently from 653d737 to 66f961c Dec 24, 2016

@mvdbeek
Copy link
Member

mvdbeek left a comment

Alright, this looks good!

@@ -1187,6 +1187,18 @@ use_interactive = True
# option (Linux) or -actimeo=0 (Solaris).
#retry_job_output_collection = 0

# In the past Galaxy would preserve its Python enivronment when running jobs (

This comment has been minimized.

@mvdbeek

mvdbeek Dec 28, 2016

Member

s/enivronment/environment/

@@ -1187,6 +1187,18 @@ use_interactive = True
# option (Linux) or -actimeo=0 (Solaris).
#retry_job_output_collection = 0

# In the past Galaxy would preserve its Python enivronment when running jobs (
# and still does for internal tools packaged with Galaxy). This behavior would
# expose Galaxy internals to tools and could result in problems when activating

This comment has been minimized.

@mvdbeek

mvdbeek Dec 28, 2016

Member

s/would expose/exposes/

# and still does for internal tools packaged with Galaxy). This behavior would
# expose Galaxy internals to tools and could result in problems when activating
# Python environments for tools (such as with Conda packaging). The default
# legacy_only will restrict this behavior just to tools identified by the Galaxy

This comment has been minimized.

@mvdbeek

mvdbeek Dec 28, 2016

Member

s/just//

by default. Only declared legacy tools have this available.
-->
<command>
python $__tool_directory__/python_environment_problem.py $out_file1

This comment has been minimized.

@mvdbeek

mvdbeek Dec 28, 2016

Member

$out_file1 should be quoted, right?

Limit adjusting Galaxy's Python environment to legacy tools.
Don't expose Galaxy's environment to tools unless there is evidence we need to. This should restrict future bugs by limitting what tools are exposed to Galaxy's internals and will fix bugs related to Conda dependencies masking Galaxy - such as #2994.

Should fix #2994.

Rebased with language and tool best practice fixes thanks to @mvdbeek.

@jmchilton jmchilton force-pushed the jmchilton:clean_python branch from 66f961c to 8d4f995 Jan 3, 2017

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Jan 3, 2017

@mvdbeek Thanks for the detailed review - I have rebased this PR and I believe I have addressed your comments.

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Jan 3, 2017

Alright, thanks @jmchilton 👍

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Jan 3, 2017

Framework and API test are passing locally as well.

@mvdbeek mvdbeek merged commit 7bac8bb into galaxyproject:dev Jan 3, 2017

2 of 4 checks passed

api test Build finished. No test results found.
Details
framework test Build finished. No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@martenson

This comment has been minimized.

Copy link
Member

martenson commented Jan 4, 2017

Thanks for addressing this @jmchilton & @mvdbeek !

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Jan 31, 2017

martenson added a commit that referenced this pull request Jan 31, 2017

Merge pull request #3521 from jmchilton/fix_sample_desc_3364
[17.01] Fix sample description bug from #3364.

peterjc added a commit to peterjc/pico_galaxy that referenced this pull request Jan 31, 2017

@peterjc

This comment has been minimized.

Copy link
Contributor

peterjc commented Feb 1, 2017

Are the implications of this change clear in the release notes for the next release?

Has anyone counted the number of tool shed Python scripts using these libraries? e.g. using grep "galaxy_utils.sequence" ...? This would give an indication of the number of Galaxy instances this change would impact, and tool authors who could be contacted.

See also http://dev.list.galaxyproject.org/Tools-using-Galaxy-s-Python-library-td4670472.html

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Feb 1, 2017

@jmchilton Would you mind putting a note into RN for this? Maybe in deprecation section or similar.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Feb 1, 2017

I can certainly add something to the release notes. My strategy was to not advertise too much at first because I really did not think this would affect anyone's tools except devteam tools. I will see what I can do.

@peterjc

This comment has been minimized.

Copy link
Contributor

peterjc commented Feb 1, 2017

Could someone with access to the Tool Shed file system run that grep to get a better idea of how many tools are affected?

@martenson

This comment has been minimized.

@peterjc

This comment has been minimized.

Copy link
Contributor

peterjc commented Feb 1, 2017

Thanks - that shows is not just me who's been using the Galaxy sequence library in their tools, but that this is a relatively minor problem. Would if be easy to pull put the owners of those repositories to contact them directly?

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Feb 1, 2017

I'll go through these in detail - but the data types are fine and so are the devteam tools that supply the dependency. That seemed to be most of the occurrences at first glance - but I'll open a PR with tool ids as I find them.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Feb 2, 2017

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Feb 2, 2017

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Feb 2, 2017

martenson added a commit that referenced this pull request Feb 2, 2017

Merge pull request #3540 from jmchilton/wiggle_fix_1701
[17.01] Fix wiggle to wiggle2simple1 for Python path changes in #3364.

martenson added a commit that referenced this pull request Feb 2, 2017

Merge pull request #3542 from jmchilton/more_python_preservation
[17.01] Fix tools that use Galaxy sequence utilities for #3364.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Feb 3, 2017

Migrate galaxy_utils to a wheel.
Alternative approach to galaxyproject#3542 which was not future proof with Conda dependencies. I think this should be now.

xref galaxyproject#3364 galaxyproject#2994 http://dev.list.galaxyproject.org/Tools-using-Galaxy-s-Python-library-td4670472.html

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Feb 3, 2017

Migrate galaxy_utils to a wheel.
Alternative approach to galaxyproject#3542 which was not future proof with Conda dependencies. I think this should be now.

xref galaxyproject#3364 galaxyproject#2994 http://dev.list.galaxyproject.org/Tools-using-Galaxy-s-Python-library-td4670472.html

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Feb 3, 2017

Migrate galaxy_utils to a wheel.
Alternative approach to galaxyproject#3542 which was not future proof with Conda dependencies. I think this should be now.

xref galaxyproject#3364 galaxyproject#2994 http://dev.list.galaxyproject.org/Tools-using-Galaxy-s-Python-library-td4670472.html

Rebased with fixes from @nsoranzo and @mvdbeek.
@natefoo

This comment has been minimized.

Copy link
Member

natefoo commented Feb 14, 2017

Man, I miss so many good PRs.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 27, 2017

Fix CONVERTER_gff_to_fli_0 for striping Galaxy from Python environment.
Taking another stab at fixing galaxyproject#3499 which was broken with galaxyproject#3364. Last attempt at fixing converters didn't cover this (galaxyproject#3690) but I missed this tool and bed to fli fixed in galaxyproject#3822.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment