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

Make conda autoinstall of packages safer: #2538

Merged
merged 12 commits into from Jul 21, 2016

Conversation

Projects
None yet
6 participants
@pvanheus
Copy link
Contributor

commented Jun 25, 2016

  • Expand check of installed environments to see if environment really contains package it should
  • Check to see if install of environment succeeds from return code
  • Remove failed environment installs

This patch is in response to this issue (#2537) but has general utility. Currently the conda dependency resolver is too confident that installation has succeeded when in fact, a broken environment was created. Thus this patch tightens up on the install checks and removes broken conda environments.

pvanheus added some commits Mar 15, 2016

Merge pull request #1 from galaxyproject/dev
pull updates from HEAD of dev
Merge pull request #2 from galaxyproject/dev
Merge pull request #2 from galaxyproject/dev
Make conda autoinstall of packages safer:
* Expand check of installed environments to see if environment really contains package it should
* Check to see if install of environment succeeds from return code
* Remove failed environment installs

@galaxybot galaxybot added the triage label Jun 25, 2016

@galaxybot galaxybot added this to the 16.07 milestone Jun 25, 2016

else:
# got to the end of the loop without finding the package
return False
else:

This comment has been minimized.

Copy link
@bgruening

bgruening Jun 25, 2016

Member

This can be simplified if you return "False" without the two else

@@ -309,7 +324,20 @@ def is_target_available(conda_target, conda_context=None):

def is_conda_target_installed(conda_target, conda_context=None):
conda_context = _ensure_conda_context(conda_context)
return conda_context.has_env(conda_target.install_environment)
if conda_context.has_env(conda_target.install_environment):
tempdir = tempfile.mkdtemp(prefix="jobdeps")

This comment has been minimized.

Copy link
@bgruening

bgruening Jun 25, 2016

Member

Do we need a directory here? Maybe an StringIO.StringIO() object would do the trick?

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jun 26, 2016

I need some time to review it and I probably won't get around to it until after the GCC - but this looks really well done. This focus on detail is awesome and improving the deployer user experience is simply fantastic.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

Very nice - thanks a bunch!

@jmchilton jmchilton merged commit 2075af4 into galaxyproject:dev Jul 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
f, package_list_file = tempfile.mkstemp(suffix='.env_packages')
os.close(f)
conda_context.export_list(conda_target.install_environment, package_list_file)
search_pattern = conda_target.package_specifier + '='

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jul 22, 2016

Member

@pvanheus
This is a bit too restrictive for my taste, since the tool maintainer specifying the version should include the ("=") if deemed necessary ... and this is in contradiction with what list_dependencies() produces :/.

Any chance you remember which package needed this more verbose checking ?

Looking over this, is it not an artefact of not checking conda's return code (which works beautifully, thanks!) ?

mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Nov 4, 2016

Remove verbose_install_check option
This option was introduced with galaxyproject#2554 and galaxyproject#2538, in reponse to a false
positive conda install.  This would have been caught by simply checking
the exit code, which we are doing by default now. The additional
verification is too strigent and not in line with the conda resolver.
Activating this option will report perfectly well installed conda
environments as having failed and will cause them to be uninstalled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.