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

Adding WDL and Docker Contribution Guides #23

Merged
merged 9 commits into from
Mar 14, 2024
Merged

Adding WDL and Docker Contribution Guides #23

merged 9 commits into from
Mar 14, 2024

Conversation

tefirman
Copy link
Member

@tefirman tefirman commented Mar 8, 2024

As the WILDS WDL pipelines prepare to make their debut in a week and a half, we need guidelines around how WDL's and their corresponding Docker images should be designed. This pull request adds initial versions for each set of guidelines and incorporates them into the overarching WILDS Contribution Guide after the "Package Maintenance" section but before the "Security" section. The location of these guides is definitely flexible, just wanted to get an initial version out there and we can move it as needed in the future. These are definitely first drafts, so if you'd like to add/remove guidelines or you feel that something's misworded/misarranged, please feel free to suggest those changes. Thanks for your input! Really appreciate it! #IntoTheWildsWeGo 🐺

Copy link

github-actions bot commented Mar 8, 2024

@github-actions github-actions bot temporarily deployed to pull request March 8, 2024 23:06 Inactive
@seankross seankross self-requested a review March 9, 2024 00:24
@tefirman tefirman requested a review from caalo March 11, 2024 17:19
Copy link
Contributor

@sitapriyamoorthi sitapriyamoorthi left a comment

Choose a reason for hiding this comment

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

Hey I think it would just be easier for me to pull a branch make recommendations and then send them over.

wdlconfig.qmd Outdated Show resolved Hide resolved
wdlconfig.qmd Outdated Show resolved Hide resolved
wdlconfig.qmd Outdated Show resolved Hide resolved
wdlconfig.qmd Outdated Show resolved Hide resolved
wdlconfig.qmd Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot temporarily deployed to pull request March 11, 2024 20:24 Inactive
@tefirman
Copy link
Member Author

@sitapriyamoorthi -- I pushed some updates to address your comments on the WDL Config file, would you mind taking a look one more time?

Copy link

@github-actions github-actions bot temporarily deployed to pull request March 11, 2024 20:54 Inactive
docker.qmd Show resolved Hide resolved
Copy link

@github-actions github-actions bot temporarily deployed to pull request March 14, 2024 17:49 Inactive
Copy link

@caalo caalo left a comment

Choose a reason for hiding this comment

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

This looks great to me.

Here are some best practices in "Developing WDL Workflows" that we are preaching that you might want to consider to incorporate: (mostly in "The first task" )

  • We prefer "heredoc" syntax in the Command section.

  • We put quotation marks around a String or File variables in Commands.

  • We recommend file localization (might be less important as this is an issue we see in miniWDL).

  • We specify in more detail what Runtime attributes are needed for each backend, and what FH HPC needs and what it will ignore. See our appendix.

Copy link

@github-actions github-actions bot temporarily deployed to pull request March 14, 2024 21:27 Inactive
@tefirman
Copy link
Member Author

Great call, @caalo, thanks for catching that! Just added a few bulletpoints to the Stylistic Guidelines section to address that and added a couple more links out to the WDL101 course in case people want to learn more.

@tefirman tefirman merged commit f1c4b87 into main Mar 14, 2024
2 checks passed
@tefirman tefirman deleted the tf-wdl branch March 14, 2024 21:44
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.

4 participants