Skip to content

[batch] Use _batch_client() to query supported_regions() and default_region()#15289

Open
jmarshall wants to merge 2 commits intohail-is:mainfrom
populationgenomics:default_region-via-_batch_client
Open

[batch] Use _batch_client() to query supported_regions() and default_region()#15289
jmarshall wants to merge 2 commits intohail-is:mainfrom
populationgenomics:default_region-via-_batch_client

Conversation

@jmarshall
Copy link
Contributor

Change Description

As default_region() is used in __init__(), it needs to work in all scenarios, including those in which credentials are provided via __init__'s token parameter. Thus it will need self._token so can't be a staticmethod, so it might as well reuse _batch_client() and its session.

Do the same with supported_regions() for good measure.

Lift self.__batch_client=None to the top so it occurs before anything might use _batch_client(), e.g., our self.default_region() call. This also ensures the __batch_client attribute exists within __del__() even if __init__() raises an exception, preventing a common red herring warning.

Security Assessment

  • This change potentially impacts the Hail Batch instance as deployed by Broad Institute in GCP

Impact Rating

  • This change has a low security impact

Impact Description

Some requests that currently fail due to being entirely unauthenticated will be tried using the authentication token the caller intends to use instead, and will have an opportunity to succeed.

Appsec Review

  • Required: The impact has been assessed and approved by appsec

As default_region() is used in __init__(), it needs to work in all
scenarios, including those in which credentials are provided via __init__'s
token parameter. Thus it will need self._token so can't be a staticmethod,
so it might as well reuse _batch_client() and its session.

Do the same with supported_regions() for good measure.

Lift self.__batch_client=None to the top so it occurs before anything might
use _batch_client(), e.g. our self.default_region() call. This also ensures
the __batch_client attribute exists within __del__() even if __init__()
raises an exception, preventing a common red herring warning.
@jmarshall jmarshall requested a review from a team as a code owner February 19, 2026 03:14
@cjllanwarne cjllanwarne self-requested a review March 11, 2026 18:25
Copy link
Collaborator

@cjllanwarne cjllanwarne left a comment

Choose a reason for hiding this comment

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

My comments are mostly "nit" level so feel free to push back. But it looks like you ran into trouble with the static analysis (unused BatchClient import) and doctest (service_backend is not defined) so I figured I might as well comment and let you decide what to do about these ones too.

@jmarshall
Copy link
Contributor Author

jmarshall commented Mar 11, 2026

trouble with the static analysis (unused BatchClient import)

Oops. I suppose now that that's unused and so not imported we could stop renaming the other BatchClient in this source file:

from hailtop.batch_client.aioclient import BatchClient as AioBatchClient
#                                                      ^^^^^^^^^^^^^^^^^

but doing that might be more confusing…

@cjllanwarne cjllanwarne added this to the 0.2.138 milestone Mar 20, 2026
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.

3 participants