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

Keep tool parameters when rerunning a job #4271

Merged
merged 4 commits into from Jul 4, 2017

Conversation

Projects
None yet
5 participants
@mvdbeek
Copy link
Member

commented Jun 30, 2017

with a different tool version, as long as the parameter did not change.
Addresses #1729

}
build_data = $.extend( {}, Galaxy.params );
build_data[ 'tool_id' ] && ( delete build_data[ 'tool_id' ] );
options.version && ( build_data[ 'tool_version' ] = options.version );

This comment has been minimized.

Copy link
@guerler

guerler Jun 30, 2017

Contributor

Changes to this file look like a bad merge.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jun 30, 2017

Author Member

@guerler I've rebased the PR (36bdd99), does this look better to you ?

@galaxybot galaxybot added this to the 17.09 milestone Jun 30, 2017

Keep tool parameters when rerunning a job
with a different tool version, as long as the parameter did not change.
Addresses #1729

@mvdbeek mvdbeek force-pushed the mvdbeek:job_rerun_changed_parameters branch from 3d45d4e to 36bdd99 Jun 30, 2017

}
options.version && ( build_data[ 'tool_version' ] = options.version );

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jun 30, 2017

Member

Seems the indentation is missing a space.

if ( new_options.job_id ) {
build_url = Galaxy.root + 'api/jobs/' + new_options.job_id + '/build_for_rerun';
if ( options.job_id ) {
build_url = Galaxy.root + 'api/jobs/' + options.job_id + '/build_for_rerun';

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jun 30, 2017

Member

These 2 changes above seem to revert the recent commit a498390 from @guerler which I think was correct, still a bad merge maybe?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jul 2, 2017

Author Member

What I have observed is that options always contains the job_id, but new_options only contains changed options. new_options only contains the job_id when clicking on the rerun icon. So the change in a498390 narrows looking up the job_id, but we now also need the job_id when having clicked on the rerun icon, followed by switching the tool version (It is not a new option anymore). So maybe it is better to use either new_options.job_id or options.job_id

@@ -418,7 +418,8 @@ def get_tool( self, tool_id, tool_version=None, get_all_versions=False, exact=Fa
if tool_version and tool_version in self._tool_versions_by_id[ tool_id ]:
return self._tool_versions_by_id[ tool_id ][ tool_version ]
# tool_id exactly matches an available tool by id (which is 'old' tool_id or guid)
return self._tools_by_id[ tool_id ]
if not tool_version:
return self._tools_by_id[ tool_id ]

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jun 30, 2017

Member

Good catch! I'd restructure lines 418-422 as:

                # tool_id exactly matches an available tool by id (which is 'old' tool_id or guid)
                if not tool_version:
                    return self._tools_by_id[ tool_id ]
                elif tool_version in self._tool_versions_by_id[ tool_id ]:
                    return self._tool_versions_by_id[ tool_id ][ tool_version ]

@jmchilton jmchilton merged commit bc8d6dd into galaxyproject:dev Jul 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. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

Great - as always thanks a ton @mvdbeek !

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