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

Allow to filter /api/invocations by job_id #11882

Conversation

scholtalbers
Copy link
Contributor

What did you do?

  • add an additional filter to /api/invocations to filter this endpoint by a dataset id

Why did you make this change?

This will allow one to retrieve the workflow invocation and subsequently the workflow (through the workflow id) that produced the given dataset.
Without this, I could only see to retrieve the responsible workflow through /datasets/<id> -> creating_job -> /jobs/<id/ -> __workflow_invocation_uuid__ -> filter invocations/ -> (..)

Alternatively, I wanted to add the workflow_id to the dataset show endpoint, but due to the additional queries/joins this might be too heavy.
In addition, I'm happy for alternative solutions to the problem "dataset_id => workflow_id".

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. run a workflow
    2. use the resulting dataset id to filter -> /api/invocations?dataset_id=

License

@github-actions github-actions bot added this to the 21.05 milestone Apr 22, 2021
if dataset_id is not None:
invocations_query = invocations_query.join(
model.WorkflowInvocationOutputDatasetAssociation
).filter(model.WorkflowInvocationOutputDatasetAssociation.table.c.dataset_id == dataset_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works for simple workflow outputs (so not for any random dataset produced by a workflow). It also doesn't work for collection. I think filtering by job is the correct way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, which is currently not yet possible right? So I could update this change from dataset_id to job_id?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be fine. There's a 1 to 1 relationship between job and invocation, so maybe this would be more appropriate in

def show_invocation(self, trans: GalaxyWebTransaction, invocation_id, **kwd):
?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, but then we need to make the invocation_id optional? Otherwise, one needs to know the invocation_id upfront..

However, the invocation_id and potentially the workflow_id could also be returned on the show job endpoint, then there is no need for a filter.

@scholtalbers scholtalbers changed the title Allow to filter /api/invocations by dataset_id WIP: Allow to filter /api/invocations by dataset_id Apr 22, 2021
@scholtalbers scholtalbers changed the title WIP: Allow to filter /api/invocations by dataset_id WIP: Allow to filter /api/invocations by job_id Apr 22, 2021
@scholtalbers scholtalbers changed the title WIP: Allow to filter /api/invocations by job_id Allow to filter /api/invocations by job_id Apr 22, 2021
@simonbray
Copy link
Member

I think this is quite similar to the API changes in #11244?

@scholtalbers
Copy link
Contributor Author

@simonbray its similar indeed. Except that on the /invocations the workflow id is returned. Whereas on the changes from #11244 jobs/<uuid>/?view=workflow only the invocation id is returned. Possibly the ?view=workflow could be updated to return this information as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants