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

Tool version and lineage fixes #4375

Merged
merged 5 commits into from Aug 4, 2017

Conversation

Projects
None yet
4 participants
@mvdbeek
Copy link
Member

commented Aug 3, 2017

  • 8d5a315 should ensure that all tools in all circumstances have a lineage and is a companion fix to #4317

  • 1369d76 specifies the tool version that should be fetched from the toolbox in more places

mvdbeek added some commits Jul 20, 2017

Always use tool version to get tools from toolbox
This fixes a bug when getting parameters for jobs that were executed
with old tool versions.
@@ -704,6 +704,7 @@ def __watch_directory( self, directory, elems, integrated_elems, load_panel_dict
def quick_load( tool_file, async=True ):
try:
tool = self.load_tool( tool_file )
tool._lineage = self._lineage_map.register( tool )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 3, 2017

Member

Maybe this can be moved inside the __add_tool() method (called on the next line and similarly at lines 569-572)?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 4, 2017

Author Member

I already tried that in the initial lineage PR ... now if only I could remember why that didn't work.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 4, 2017

Author Member

It does seem to work, so this is definitely an improvement!

@galaxybot galaxybot added this to the 17.09 milestone Aug 3, 2017

@@ -846,7 +846,7 @@ def get_param_values( self, app ):
dict of tool parameter values.
"""
param_dict = dict( [ ( p.name, p.value ) for p in self.parent_job.parameters ] )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 3, 2017

Member

Not connected with your changes, but I think self.parent_job should be self.job .
Introduced in commit 988d94f, ping @dannon.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 4, 2017

Author Member

That could be why @bgruening reported issues with parallelism tools ...

@@ -846,7 +846,7 @@ def get_param_values( self, app ):
dict of tool parameter values.
"""
param_dict = dict( [ ( p.name, p.value ) for p in self.parent_job.parameters ] )
tool = app.toolbox.get_tool( self.tool_id )
tool = app.toolbox.get_tool( self.tool_id, tool_version=self.job.tool_version )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 3, 2017

Member

I think that self.tool_id should be changed to self.job.tool_id . I suspect this method may be unused.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 4, 2017

Author Member

I think you're right. The Task thing is for parallelism="true" and split_inputs="input1,input2,..." tools, right ? I don't think we have many of these, so I suppose it's possible we never noticed this.

This comment has been minimized.

Copy link
@mvdbeek
@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

@mvdbeek Do you also want to revert #4317 as part of this PR?

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2017

I principle that would be good, but I haven't been able to reproduce the issue :/
I guess it's better to keep it in.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

Then what about adding a log.warning() as suggested in #4317 (comment) ?

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2017

Yep, done that in ccad70c

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

Fantastic, thanks a lot!

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

Thank you very much @mvdbeek

@martenson martenson merged commit 575696d into galaxyproject:dev Aug 4, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 37 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

@mvdbeek mvdbeek deleted the mvdbeek:tool_version_and_lineage_fixes 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.