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

[18.01] Implement more intuitive collection PJA #5418

Merged
merged 1 commit into from Feb 3, 2018

Conversation

Projects
None yet
4 participants
@jmchilton
Copy link
Member

jmchilton commented Jan 30, 2018

Extends work for rename from #5416 to add PJA implementations for output collections and implicit mapped over output collections for the PJA types "add tag", "remove tag", "delete", and "hide".

WIP because this all needs tests. Now with tests!

Fixes #1680.

@jmchilton jmchilton changed the base branch from dev to release_18.01 Jan 30, 2018

@jmchilton jmchilton changed the title [WIP] Implement collection PJA [18.01][WIP] Implement collection PJA Jan 30, 2018

if tags:
for name, step_output in step_outputs.items():
if action.output_name == '' or name == action.output_name:
cls._execute(app, sa_session.user, step_output, tags)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Jan 30, 2018

Author Member

Well that isn't right @jmchilton. Like you said - needs tests.

type: File
name: fasta1
""", history_id=history_id)
details1 = self.dataset_populator.get_history_collection_details(history_id, hid=1, wait=True, assert_ok=True)

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Feb 1, 2018

Member

Same issue as in #5416 (comment)

@martenson martenson removed the triage label Feb 1, 2018

@jmchilton jmchilton force-pushed the jmchilton:collection_pja branch 2 times, most recently from 347cf60 to 4365aee Feb 2, 2018

@jmchilton jmchilton changed the title [18.01][WIP] Implement collection PJA [18.01] Implement collection PJA Feb 2, 2018

@jmchilton jmchilton changed the title [18.01] Implement collection PJA [18.01] Implement more intuitive collection PJA Feb 2, 2018

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Feb 2, 2018

Rebased with tests - taking it out of WIP. Will add a review tag if the tests pass.

value: 1.fastq
type: File
""", history_id=history_id)
details1 = self.dataset_populator.get_history_collection_details(history_id, hid=4, wait=True, assert_ok=True)

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Feb 2, 2018

Member

that should be hid 3 ... or the last hdca ? This is the history content:

[{u'create_time': u'2018-02-02T17:35:54.400833',
  u'dataset_id': u'adb5f5c93f827949',
  u'deleted': False,
  u'extension': u'txt',
  u'hid': 1,
  u'history_content_type': u'dataset',
  u'history_id': u'adb5f5c93f827949',
  u'id': u'adb5f5c93f827949',
  u'name': u'Test Dataset',
  u'purged': False,
  u'state': u'ok',
  u'tags': [],
  u'type': u'file',
  u'type_id': u'dataset-adb5f5c93f827949',
  u'update_time': u'2018-02-02T17:36:08.077169',
  u'url': u'/api/histories/adb5f5c93f827949/contents/adb5f5c93f827949',
  u'visible': True},
 {u'collection_type': u'list',
  u'deleted': False,
  u'element_count': 1,
  u'hid': 2,
  u'history_content_type': u'dataset_collection',
  u'history_id': u'adb5f5c93f827949',
  u'id': u'adb5f5c93f827949',
  u'job_source_id': None,
  u'job_source_type': None,
  u'model_class': u'HistoryDatasetCollectionAssociation',
  u'name': u'the_dataset_list',
  u'populated': True,
  u'populated_state': u'ok',
  u'populated_state_message': None,
  u'tags': [],
  u'type': u'collection',
  u'url': u'/api/histories/adb5f5c93f827949/contents/dataset_collections/adb5f5c93f827949',
  u'visible': True},
 {u'collection_type': u'list',
  u'deleted': False,
  u'element_count': 1,
  u'hid': 3,
  u'history_content_type': u'dataset_collection',
  u'history_id': u'adb5f5c93f827949',
  u'id': u'529fd61ab1c6cc36',
  u'job_source_id': u'adb5f5c93f827949',
  u'job_source_type': u'ImplicitCollectionJobs',
  u'model_class': u'HistoryDatasetCollectionAssociation',
  u'name': u'Concatenate datasets (for test workflows) on collection 2',
  u'populated': True,
  u'populated_state': u'ok',
  u'populated_state_message': None,
  u'tags': [u'name:foo'],
  u'type': u'collection',
  u'url': u'/api/histories/adb5f5c93f827949/contents/dataset_collections/529fd61ab1c6cc36',
  u'visible': True},
 {u'create_time': u'2018-02-02T17:36:07.781297',
  u'dataset_id': u'529fd61ab1c6cc36',
  u'deleted': False,
  u'extension': u'txt',
  u'hid': 4,
  u'history_content_type': u'dataset',
  u'history_id': u'adb5f5c93f827949',
  u'id': u'529fd61ab1c6cc36',
  u'name': u'Concatenate datasets (for test workflows) on data 1',
  u'purged': False,
  u'state': u'ok',
  u'tags': [u'name:foo'],
  u'type': u'file',
  u'type_id': u'dataset-529fd61ab1c6cc36',
  u'update_time': u'2018-02-02T17:36:19.915445',
  u'url': u'/api/histories/adb5f5c93f827949/contents/529fd61ab1c6cc36',
  u'visible': False}]

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 2, 2018

Author Member

Yup - redid the math and that makes sense. Sorry.

Implement more collection workflow post job actions.
Implement hide, delete, add tag, and remove tag for output collections and mapped over collections.

Fixes #1680.

@jmchilton jmchilton force-pushed the jmchilton:collection_pja branch from 4365aee to 8e32e93 Feb 2, 2018

@@ -242,6 +252,16 @@ def execute(cls, app, sa_session, action, job, replacement_dict):
if action.output_name == '' or dataset_assoc.name == action.output_name:
dataset_assoc.dataset.deleted = True

for dataset_collection_assoc in job.output_dataset_collection_instances:

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Feb 3, 2018

Member

Hmm, I like the DeleteDatasetAction option, but as per the first comment it says this is disabled ?!
I think it's not actually disabled, but I have experienced situations where intermediate data got deleted too early ... I'll see if I can write a testcase for that.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Feb 3, 2018

Member

Alright, I get it, it is disabled because it's not listed in the ActionBox

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Feb 3, 2018

Member

I confused this with Output cleanup/DeleteIntermediatesAction , I think, so that's unrelated.

@@ -389,8 +423,8 @@ class RemoveTagDatasetAction(TagDatasetAction):
direction = "from"

@classmethod
def _execute(cls, app, user, dataset, tags):
app.tag_handler.remove_tags_from_list(user, dataset, tags)
def _execute(cls, app, user, output, tags):

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Feb 3, 2018

Member

here we're missing the execute_on_mapped_over classmethod, right ?

Nevermind, it's inherited from TagDatasetAction

@mvdbeek

mvdbeek approved these changes Feb 3, 2018

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Feb 3, 2018

This is absolutely amazing! This is one of the (maybe not so) small things that I've just learned to live with, back when I didn't even think I could program. @mblue9 @lparsons I think this also addresses a few shortcomings we've discussed. Now hiding of collections and removing/adding tags to collections via workflows works properly in more instances than it did before.

@mvdbeek mvdbeek merged commit 5aed347 into galaxyproject:release_18.01 Feb 3, 2018

6 checks passed

api test Build finished. 349 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 171 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 79 tests run, 4 skipped, 0 failed.
Details
selenium test Build finished. 118 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Feb 3, 2018

We should mention this in the release notes!

@galaxybot

This comment has been minimized.

Copy link

galaxybot commented Feb 3, 2018

This PR was merged without a milestone attached.

@mvdbeek mvdbeek added this to the 18.01 milestone Feb 3, 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.