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

feat: implement JSON inputs files in wdl-engine. #241

Merged
merged 7 commits into from
Oct 30, 2024

Conversation

peterhuene
Copy link
Collaborator

@peterhuene peterhuene commented Oct 30, 2024

This PR implements support for JSON input files in wdl-engine.

The file format conforms to the WDL specification for JSON input files.

Value coercion now returns errors for so users can better diagnose coercion
problems.

Also adds stub methods to Engine for evaluating workflows and tasks; the
implementation is forthcoming.

Before submitting this PR, please make sure:

  • You have added a few sentences describing the PR here.
  • You have added yourself or the appropriate individual as the assignee.
  • You have added at least one relevant code reviewer to the PR.
  • Your code builds clean without any errors or warnings.
  • You have added tests (when appropriate).
  • You have updated the README or other documentation to account for these
    changes (when appropriate).
  • You have added an entry to the relevant CHANGELOG.md (see
    "keep a changelog" for more information).
  • Your commit messages follow the conventional commit style.

This commit adds a types collection `Engine`, making it clear that any types
exposed via the `Engine` API come from that singular collection.
This commit implements support for JSON input files in `wdl-engine`.

The file format conforms to the WDL specification for JSON input files.

Also adds stub methods to `Engine` for evaluating workflows and tasks; the
implementation is forthcoming.
Copy link
Member

@claymcleod claymcleod left a comment

Choose a reason for hiding this comment

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

Looks good to me!

wdl-analysis/src/types/v1.rs Outdated Show resolved Hide resolved
/// The overridden requirements section values.
requirements: HashMap<String, Value>,
/// The overridden hints section values.
hints: HashMap<String, Value>,
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize you could override hints. What's the use case for that? Spitballing: I guess it makes sense if an implementation leaves out a hints section and you want to "fill it in" at runtime. But overriding specified values? That seems wrong to me, but maybe I'm just not thinking of the right use case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, both requirements and hints can be overridden and an engine must support doing so.

Probably useful for when you might have a bespoke container image you want to run someone else's task in or the environment you'll be running a task on would benefit from higher minimum/maximum cpu/memory/storage.

Copy link
Member

Choose a reason for hiding this comment

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

We've written workflows in a way that a user should never have to override a runtime/requirements value (or at least it should be rare), but that use case makes perfect sense to me. A task is failing due to too low a mem allocation? Override runtime.memory.

But hints??? Those don't seem like something that should be "messed with" by the user. But I haven't actually written any hints sections for production, so maybe I'm just thinking about it wrong.

@adthrasher just curious if you have thoughts on this. Seems an odd case to me.

wdl-engine/src/inputs.rs Outdated Show resolved Hide resolved
wdl-engine/tests/inputs/invalid-hints-type/source.wdl Outdated Show resolved Hide resolved
peterhuene and others added 3 commits October 30, 2024 10:11
Calculate display strings for requirement and hint types rather than using a
hardcoded string.
Copy link
Member

@a-frantz a-frantz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@peterhuene peterhuene merged commit f68bbc5 into stjude-rust-labs:main Oct 30, 2024
12 checks passed
@peterhuene peterhuene deleted the engine branch October 30, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants