-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update GCP docs #59
Update GCP docs #59
Conversation
Minimum allowed coverage is Generated by 🐒 cobertura-action against 400accc |
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.
Looks good, thanks Natalie!
docs/project_defn.rst
Outdated
* ``batch_array_size``: Number of tasks to divide the simulations into. Max: 10000. | ||
(e.g. ``us-central1``) | ||
* ``batch_array_size``: Number of tasks to divide the simulations into. Tasks with fewer than 100 | ||
simulations each are recommended, especially when using spot instances. Max: 10,000. |
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.
to avoid loosing too much of simulations or anything in respect to that when preemption happens? maybe adding something like this to justify the recommendation on < 100 sims per task?
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.
Added.
* ``use_spot``: true or false. This tells the project whether to use | ||
`Spot VMs <https://cloud.google.com/spot-vms>`_ for data simulations, which can reduce | ||
costs by up to 91%. Default: false | ||
* ``use_spot``: Optional. Whether to use `Spot VMs <https://cloud.google.com/spot-vms>`_ |
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.
maybe we should change the default to true here? thoughts?
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 think I prefer a default of False, because 1) that's the default when creating GCP jobs directly and 2) I think it's best for the default to be the most reliable option, at least as long as using spot instances at scale continues to require manual retries sometimes.
docs/project_defn.rst
Outdated
* ``memory_mib``: `Amount of RAM`_ needed in MiB. 2048 MiB per CPU is recommended. Default: | ||
4096. | ||
* ``cpus``: Optional. `Number of CPUs`_ to use. Default: 2. | ||
* ``memory_mib``: Optional. `Amount of RAM`_ needed in MiB. 2048 MiB per CPU is 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.
probably saying "at least 2048 MiB per cpu 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.
do we want to mention anything about the guardrails here?
You can download and preview the docs changes at Checks -> Artifacts -> documentation, if you want to see exactly what the html pages will look like.