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

Migrate the route "/api/invocations/{invocation_id}" of the workflows API to Fast API #17052

Closed

Conversation

heisner-tillman
Copy link
Contributor

@heisner-tillman heisner-tillman commented Nov 18, 2023

This is a part of #10889.

Summary

  • Refactored API route
  • Annotated input parameters
  • Operation now returns pydantic model
  • Removed the mapping to the legacy route

How to test the changes?

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@heisner-tillman heisner-tillman marked this pull request as draft November 18, 2023 15:33
@github-actions github-actions bot added this to the 23.2 milestone Nov 18, 2023
@martenson
Copy link
Member

martenson commented Nov 19, 2023

Hi @heisner-tillman, I did not get a closer look at your PR yet but please be aware of potential duplicate effort due to #16707.

FAILED = "failed"


class EncodedInvocation(BaseModel):
Copy link
Member

Choose a reason for hiding this comment

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

Can you name these things {Something}Response ?

Comment on lines +193 to +203
model_class: str = Field(default=Required, title="Model class", description="Model class name.")
# TODO - Add proper models
steps: Optional[List[Dict[str, Optional[str]]]] = None
inputs: Optional[Dict[str, Dict[str, Optional[str]]]] = None
input_step_parameters: Optional[Dict[str, Dict[str, Optional[str]]]] = None
outputs: Optional[Dict[str, Dict[str, Optional[str]]]] = None
output_collections: Optional[Dict[str, Dict[str, Optional[str]]]] = None
output_values: Optional[Dict[str, Optional[str]]] = None
# TODO understand where this comes from
message: Optional[str] = Field(
default=None, title="Message", description="Message associated with this invocation."
Copy link
Member

Choose a reason for hiding this comment

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

Are these really optional, or is it just that they can be null ? That's not the same

@heisner-tillman
Copy link
Contributor Author

To avoid duplication, this work will be continued in collaboration with @martenson on his pull request #16707!

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

Successfully merging this pull request may close these issues.

3 participants