Cache depencencies on the fly when first used by abretaud · Pull Request #3348 · galaxyproject/galaxy

abretaud · 2016-12-21T08:56:26Z

Hi,
A tiny patch to allow caching (conda) dependencies on the fly when a tool is executed the first time (well, anytime the cache is not found in fact).

There is some discussion about this in galaxyproject/planemo#612

Just tested it with planemo, it seems to work fine

ping @mvdbeek @jmchilton

🎅 🎅 🎅

mvdbeek · 2016-12-21T09:23:24Z

lib/galaxy/tools/deps/__init__.py

        cacheable_dependencies = [dep for dep in resolved_dependencies.values() if dep.cacheable]
        hashed_dependencies_dir = self.get_hashed_dependencies_path(cacheable_dependencies)
-        if os.path.exists(hashed_dependencies_dir):
+        if not os.path.exists(hashed_dependencies_dir):


Could you add a check for conda_auto_install here? I am very much afraid of triggering this at random (whenever these dependencies are being resolved for the first time...) timepoints on production instances.

mvdbeek · 2016-12-21T09:25:14Z

lib/galaxy/tools/deps/__init__.py

+        if not os.path.exists(hashed_dependencies_dir):
+            # Cache not present, try to create it
+            self.build_cache(requirements, **kwds)
+        if os.path.exists(hashed_dependencies_dir): # Check caching was successfull


You'll need one more space before the comment here.

mvdbeek · 2016-12-21T09:29:43Z

If you could add the check for conda_auto_install you have my 👍.
Though ideally I would prefer to see a separate setting for this behavior, i.e. build_cache_on_first_run or sth. like that, given that the cached environments are -- in principle -- not bound to conda, so it's a bit counter-intuitive to couple this to conda_auto_install. We could still make build_cache_on_first_run default to True if conda_auto_install is set to true.

abretaud · 2016-12-21T10:16:31Z

Ok, I added the build_cache_on_first_run option (hope it won't make too many options for conda related features!)
I'm hesitant to make it default to True if conda_auto_install is True. It seems a cool behavior, but I fear it would bring some confusion (ie I set it to False in galaxy.ini but it is magically set to True internally)

For planemo, I guess we could set build_cache_on_first_run to True when --use_cached_dependency_manager?

mvdbeek · 2016-12-21T10:17:09Z

That looks very useful, thanks for looking into this 👍

mvdbeek · 2016-12-21T10:22:35Z

I'm hesitant to make it default to True if conda_auto_install is True. It seems a cool behavior, but I fear it would bring some confusion (ie I set it to False in galaxy.ini but it is magically set to True internally)

For planemo, I guess we could set build_cache_on_first_run to True when --use_cached_dependency_manager?

You're right, this might be a bit surprising. I think that for planemo we could make use_cached_dependency_manager and build_cache_on_first_run the default if conda_auto_install is specified.

mvdbeek · 2016-12-21T10:33:23Z

lib/galaxy/tools/deps/__init__.py

        resolved_dependencies = self.requirements_to_dependencies(requirements, **kwds)
        cacheable_dependencies = [dep for dep in resolved_dependencies.values() if dep.cacheable]
        hashed_dependencies_dir = self.get_hashed_dependencies_path(cacheable_dependencies)
+        if not os.path.exists(hashed_dependencies_dir) and self.extra_config['build_cache_on_first_run'] == True:


Travis is complaining here: comparison to True should be 'if cond is True:' or 'if cond:'

jmchilton · 2016-12-21T18:12:07Z

I think I like the direction we are heading here - and I certainly think y'all are correct that planemo should be used to setup these defaults correctly if --conda_auto_install.

I do have two changes I'd like to at least throw out there.

I feel like precache_dependencies would be a better name.
I also feel like the default should be True, though maybe I'm wrong? If you are using a cached dependency manager you want the dependencies cached right?

nsoranzo · 2016-12-21T20:25:16Z

I think we already have many options for Conda and this one is not needed.

I am very much afraid of triggering this at random (whenever these dependencies are being resolved for the first time...) timepoints on production instances.

@mvdbeek Building the cached env probably takes the same time of creating the Conda env for the job (which is later destroyed), no?

jmchilton · 2016-12-21T20:48:06Z

@nsoranzo I disagree a bit - if it defaults to True no one needs to worry about it by default. There are scenarios where you want to disable it - for instance if you want to have your dependencies mounted read-only (very reasonable if your trying to lock things down for security reasons).

nsoranzo · 2016-12-21T21:02:48Z

@nsoranzo I disagree a bit - if it defaults to True no one needs to worry about it by default. There are scenarios where you want to disable it - for instance if you want to have your dependencies mounted read-only (very reasonable if your trying to lock things down for security reasons).

Fair enough, +1 if it defaults to True.

mvdbeek · 2016-12-21T21:20:03Z

Building the cached env probably takes the same time of creating the Conda env for the job (which is later destroyed), no?

That's true ... I guess it's not that big a deal from this angle.
I was just thinking about bioperl, which takes a good 15 minutes to build, but it's true that that would also happen if it's set to false.

bgruening · 2017-01-01T18:13:03Z

@abretaud great addition!

cache deps on the fly when first used

74dfcd3

abretaud mentioned this pull request Dec 21, 2016

Add --use_cached_dependency_manager option galaxyproject/planemo#612

Merged

galaxybot added the triage label Dec 21, 2016

galaxybot added this to the 17.01 milestone Dec 21, 2016

mvdbeek reviewed Dec 21, 2016

View reviewed changes

add build_cache_on_first_run option

9dbe766

mvdbeek added area/admin area/tool-dependencies area/tools kind/enhancement status/review and removed triage labels Dec 21, 2016

mvdbeek reviewed Dec 21, 2016

View reviewed changes

make travis happy

26ba670

change option name and default value

1a0dde8

mvdbeek approved these changes Dec 28, 2016

View reviewed changes

nsoranzo approved these changes Dec 28, 2016

View reviewed changes

bgruening merged commit 408254f into galaxyproject:dev Jan 1, 2017

Conversation

abretaud commented Dec 21, 2016

Uh oh!

mvdbeek Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

mvdbeek Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

mvdbeek commented Dec 21, 2016

Uh oh!

abretaud commented Dec 21, 2016

Uh oh!

mvdbeek commented Dec 21, 2016

Uh oh!

mvdbeek commented Dec 21, 2016

Uh oh!

mvdbeek Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

jmchilton commented Dec 21, 2016

Uh oh!

nsoranzo commented Dec 21, 2016

Uh oh!

jmchilton commented Dec 21, 2016

Uh oh!

nsoranzo commented Dec 21, 2016

Uh oh!

mvdbeek commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgruening commented Jan 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mvdbeek commented Dec 21, 2016 •

edited

Loading