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

Add API and UI to remove requirements installed by Conda #3725

Merged
merged 6 commits into from Mar 13, 2017

Conversation

Projects
None yet
4 participants
@mvdbeek
Copy link
Member

commented Mar 8, 2017

This is currently limited to the Conda dependency resolver, but other
resolver would only need to implement the resolver.uninstall method and
set can_uninstall_dependencies to True.

To use this endpoint, DELETE to
<galaxy_url>/tools/<tool_id>/dependencies.

In this PR I have also added the possibility to POST to <galaxy_url>/tools/<tool_id>/dependencies, which will install the dependencies, in favor of the older <galaxy_url>/tools/<tool_id>/install_dependencies endpoint which we will deprecate (when ?).

If this is accepted, I will enhance the new manage tool dependencies page so that we can remove and reinstall dependencies from there.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

@mvdbeek Are you willing to work on these endpoint URLs a bit?

I'd like to maintain

POST /api/tools/{tool_id}/install_dependencies

for backward compat. but I guess going forward I'd like to see the following for install and uninstall:

POST /api/tools/{tool_id}/dependencies
DELETE /api/tools/{tool_id}/dependencies

It feels more RESTful to me.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2017

Oh, sure, that makes sense!

@jmchilton

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

Did you mean to add #3729 into this PR?

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2017

damnit, too much switching branches :(

@mvdbeek mvdbeek force-pushed the mvdbeek:remove_env_conda branch 2 times, most recently from 9c9d407 to 056b20f Mar 9, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2017

Alright, this should be OK now.

@martenson
Copy link
Member

left a comment

looks great

@martenson

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

should we add POST /api/tools/{tool_id}/install_dependencies to #3280 ?

@jmchilton

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

@martenson I'd say -0 on deprecation - the amount of technical debt we reduce is not worth the effort and the cost of not maintaining backward compat.

Update: I mean I'm fine if we all agree it is deprecated - I just don't think it is worth removing per se.

@martenson

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

@jmchilton not worth removing - worth announcing though?

mvdbeek added some commits Mar 8, 2017

Add API to remove requirements installed by Conda
This is currently limited to the Conda dependency resolver, but other
resolver would only need to implement the resolver.uninstall method and
set can_uninstall_dependencies to True.

To use this endpoint, POST to
<galaxy_url>/tools/<tool_id>/uninstall_dependencies'.
Change endpoint to `api/tools/<tool_id>/dependencies`
A POST will trigger installation and DELETE will trigger uninstalling
the dependencies (Thx for the suggestion @jmchilton).

@mvdbeek mvdbeek force-pushed the mvdbeek:remove_env_conda branch from 056b20f to 95106ea Mar 12, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2017

I have fixed a small problem in 6902e62 and I have added a button to actually uninstall the conda env in the UI in 95106ea.

@mvdbeek mvdbeek changed the title Add API to remove requirements installed by Conda Add API and UI to remove requirements installed by Conda Mar 12, 2017

@martenson martenson closed this Mar 12, 2017

@martenson martenson reopened this Mar 12, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

Thanks @mvdbeek !

@martenson martenson merged commit fc1df7b into galaxyproject:dev Mar 13, 2017

5 checks passed

api test Build finished. 263 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 140 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 25 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
final_return_code = 0
for env, return_code in zip(environments, return_codes):
if return_code == 0:
log.debug("Conda environment '%s' sucessfully removed." % env)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 13, 2017

Member

s/sucessfully/successfully/

@@ -116,6 +117,25 @@ def get_option(name):
def clean(self, **kwds):
return self.conda_context.exec_clean()

def uninstall(self, requirements):
"""Uninstall requirements installed by install_all or multiple install statements."""
all_resolved = [r for r in self.resolve_all(requirements) if r.dependency_type]

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 13, 2017

Member

This will break when self.resolve_all() returns False in line 163.

mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Mar 13, 2017

Return empty list if resolve_all() has failed
This should solve the problems pointed out by 👀 @nsoranzo
introduced in galaxyproject#3725.
This commit also clarifies this behaviour in the docstrings.

mvdbeek added a commit to bardin-lab/galaxy that referenced this pull request Mar 20, 2017

Return empty list if resolve_all() has failed
This should solve the problems pointed out by 👀 @nsoranzo
introduced in galaxyproject#3725.
This commit also clarifies this behaviour in the docstrings.

@mvdbeek mvdbeek deleted the mvdbeek:remove_env_conda branch Jun 12, 2018

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.