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

[hailtop.batch] add default_regions to hb.Batch, improve docs #14224

Merged
merged 7 commits into from Feb 2, 2024

Conversation

danking
Copy link
Collaborator

@danking danking commented Jan 30, 2024

hb.Batch now supports default_regions which completes the natural hierarchy of: config, envvar, backend, batch, job. I went a little hog wild with examples. I think we should have more examples everywhere!

The ServiceBackend doc page also had several basic formatting issues which I addressed.

`hb.Batch` now supports `default_regions` which completes the natural hierarchy of: config, envvar,
backend, batch, job.

The ServiceBackend doc page also had several basic formatting issues.
@@ -413,42 +413,96 @@ async def _async_close(self):


class ServiceBackend(Backend[bc.Batch]):
ANY_REGION: ClassVar[List[str]] = ['any_region']

"""Backend that executes batches on Hail's Batch Service on Google Cloud.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was actually the doc-string for the ANY_REGION class variable. By moving the class variable down, everything started rendering properly again.

Cloud regions in which jobs may run. :attr:`.ServiceBackend.ANY_REGION` indicates jobs may
run in any region. If unspecified or ``None``, the ``batch/regions`` Hail configuration
variable is consulted. See examples above. If none of these variables are set, then jobs may
run in any region. :meth:`.ServiceBackend.supported_regions` lists the available regions.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The py:attribute: stuff didn't seem to have any effect, it just rendered as text. These generate correct links.

@danking
Copy link
Collaborator Author

danking commented Jan 30, 2024

Please clone and build the docs (make -C hail batch-docs && (cd hail/build/www && python3 -m http.server) and make sure they look good!

@jigold
Copy link
Collaborator

jigold commented Jan 31, 2024

@danking Do you want me to adopt this PR? It looks like there's a bunch of annoying errors to fix with the doctests as well as maybe a missing field in the spec?

jigold
jigold previously requested changes Feb 1, 2024
hail/python/hailtop/batch/backend.py Outdated Show resolved Hide resolved
>>> b = hb.Batch(
... backend=hb.ServiceBackend(
... gcs_bucket_allow_list=['cold-bucket', 'cold-bucket2'],
... ),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
... ),
... )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is valid Python and the preferred style of the ruff formatter we're now using.

hail/python/hailtop/batch/docs/service.rst Outdated Show resolved Hide resolved
@danking danking dismissed jigold’s stale review February 1, 2024 22:28

good grammar & spelling catches

@danking
Copy link
Collaborator Author

danking commented Feb 1, 2024

I think it's passing fine now. Just a transient failure in the latest one: https://ci.hail.is/batches/8118694

@danking danking merged commit dace919 into hail-is:main Feb 2, 2024
8 checks passed
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.

None yet

2 participants