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

Do not recurse in ensure_installed() #4049

Merged
merged 4 commits into from May 16, 2017

Conversation

Projects
None yet
5 participants
@bernt-matthias
Copy link
Contributor

commented May 10, 2017

There was a missing parameter in the recursive function call.

There are two more places in the galaxy code where the parameter is missing:
lib/galaxy/tools/deps/container_resolvers/mulled.py
lib/galaxy/tools/deps/mulled/mulled_build.py

I don't know how to fix them.

Note that, the recursive function call happens when there is a file lock on a conda related file (database/dependencies/conda.lock) which appeared in my case when galaxy crashed / was aborted. It would be nice to write something to the user otherwise he will wait until the max recursion limit and get a confusing exception.

@galaxybot galaxybot added the triage label May 10, 2017

@galaxybot galaxybot added this to the 17.09 milestone May 10, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member

commented May 10, 2017

I think the other instances are fine, since they are delegating to ensure_installed in lib/galaxy/tools/deps/mulled/mulled_build.py, which does this:

def ensure_installed(involucro_context, auto_init):
    return installable.ensure_installed(involucro_context, install_involucro, auto_init)
@nsoranzo

This comment has been minimized.

Copy link
Member

commented May 10, 2017

Introduced in commit 5a72449 .

Note that, the recursive function call happens when there is a file lock on a conda related file (database/dependencies/conda.lock) which appeared in my case when galaxy crashed / was aborted. It would be nice to write something to the user otherwise he will wait until the max recursion limit and get a confusing exception.

I think a for loop would be better here, something like:

for i in range(MAX_TRIES):
    try:
        ...
    except FileLockException:
        pass  # or time.sleep(1)
raise Exception("Failed to get file lock for %s times" % MAX_TRIES)
@bernt-matthias

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

I just checked the FileLock class in the galaxy/util. The function already implements a timeout=10 and delay=.05. So the repetition is already done there and I guess we can skip it here.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented May 10, 2017

@bernt-matthias Good catch! We may want to increase the timeout though, @mvdbeek?

@nsoranzo nsoranzo changed the title added missing parameter to function ensure_installed Do not recurse in ensure_installed() May 10, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member

commented May 10, 2017

Yes, 10 seconds is quite short. This will be called when starting up galaxy (with conda_auto_init) while conda is being installed to prevent multiple handlers from installing conda at the same time. So in that case failing early is fine, but I'm not sure about invulcro/mulled.

@bernt-matthias

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

Any idea what would be a good number? 1 minute? I guess that this will happen only seldom so it wont hurt to have a large number. But it might be annoying for the user to wait for a long time for the error message. I have to little experience to come up with a good guesstimate.

@martenson

This comment has been minimized.

Copy link
Member

commented May 16, 2017

@galaxybot test this

@martenson

This comment has been minimized.

Copy link
Member

commented May 16, 2017

Thank you for the contribution @bernt-matthias !

@martenson martenson merged commit 18349b1 into galaxyproject:dev May 16, 2017

4 of 5 checks passed

api test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 148 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
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.