-
Notifications
You must be signed in to change notification settings - Fork 9
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
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?
adamrtalbot
left a comment
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 <12817534+adamrtalbot@users.noreply.github.com>
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
gwright99
left a comment
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.
In general good. I'm requesting a few prose additions because of events that came up post the session that spawned this original documentation.
| 1. The frozen image serves many task executions. | ||
|
|
||
| With freeze enabled, only the first API call to Wave counts toward your quota. | ||
| Wave reuses frozen images as long as the image and its configuration remain the same. |
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.
We should specify exactly what this is (specifically the configuration part, making sure to call out bin scripts vs module bin scripts vs task definitions). @robsyme @bentsherman @gwright99 had a follow-up convo about this and discovered the mechanics didn't quite work the way that consensus expected.
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.
Same as above, can I have some more context here please? Is there a recording or other thread I can review?
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.
Addressing in #933
|
Hi @gwright99 As per the EDU session last week, I'd like to finalize/merge this PR. As discussed, I will address the issues around builds and Wave in a separate PR. This will give me time to gather all the necessary information and determine the best place to add it. Feel free to add any additional comments here for prosperity; I'll tag you in PR's on this topic so you can see/comment on progress. |
gavinelder
left a comment
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.
General comments around the nature of this document.
It feels more like a blog post vs part of the documentation.
Maybe this should like in a "guides" section based on the current structure / format
Thanks fot the feedback @gavinelder This will sit under a 'Guides' section (not shown here, as the sidebar is currently stored in the Seqera docs repository). I agree that it's trending towards a blog; however, this was to address a specific user problem, will likely be broadly applicable, and will likely evolve over time, so I think it meets the bar for a guide. Your other points are fair enough. I'll address them shortly. I hope to expand the conceptual content for Wave to explain more of the why and how in H1 next year, so some of this may be revisited. |
|
Summary of changes Changed:
Not changed:
cc @gavinelder |
gavinelder
left a comment
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.
This looks pretty good to me I provided some feedback on the logic flows as I believe Graham's comments still stands around clarity and I feel like we could potentially replace these steps with a paragraph covering the purpose / benefit instead.
Maybe something like "with freeze your head job communicates with Wave once per container, without freeze all tasks communicate with wave for every container which may result in thosands of requests"
|
Thanks Gavin - I've pushed more changes. Some were more complex so I've pushed them directly to the PR: Changed:
Not changed:
Open questions:
|
gavinelder
left a comment
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.
Overall this looks good to me.
I have some small editorial notes but they should be considered non blocking and optional.
Co-authored-by: Gavin <gav.elder@gmail.com>
|
Thanks @gavinelder |
gwright99
left a comment
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.
Approving to get this PR unblocked.
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.