-
Notifications
You must be signed in to change notification settings - Fork 7
docs: Add Wave api rate limit guide #914
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
base: master
Are you sure you want to change the base?
Conversation
docs/nextflow/reduce-api-calls.md
Outdated
| - Lowest latency for AWS workloads | ||
| - Simplest setup | ||
|
|
||
| **Not recommended**: Private Docker Hub for AWS Batch workloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? Let's give them a reason (i.e. requires tinkering with Batch instances to authenticate to non-ECR registry).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see you did it below. nevermind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be tempted to generalise it to say "use the native cloud container registry", which has all the same benefits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've softened to "Not recommended: External container registries for AWS Batch workloads" as per your other suggestion.
Are you suggesting we don't specifically call out AWS? And just refer to "native cloud container registries" instead of "AWS Batch workloads" for both recommended and not recommended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a bit more precision around the language of manfiest vs layers, and when you access the Wave service.
docs/nextflow/reduce-api-calls.md
Outdated
| - Lowest latency for AWS workloads | ||
| - Simplest setup | ||
|
|
||
| **Not recommended**: Private Docker Hub for AWS Batch workloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be tempted to generalise it to say "use the native cloud container registry", which has all the same benefits.
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Adam Talbot <[email protected]>
|
Thanks everyone - sorry for all the noise on the PR. I've made the suggested changes, and a larger restructure after Adams' comments made me realize we had sections that were largely similar (Building without Wave freeze, Building with Wave freeze, First pipeline run, and Subsequent pipeline runs). This has now been shaped into two sections (Building without Wave freeze and Building with Wave freeze). There are two open comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Some clarification around definitions and what counts against what.
- Apply direct suggestions Co-authored-by: Adam Talbot <[email protected]>
Netlify preview is here: https://deploy-preview-845--seqera-docs.netlify.app/wave/nextflow/reduce-api-calls
Note: I don't love the location in the sidebar, but I think it's acceptable while we consider how guides might be presented in the docs moving forward.