Conversation
|
|
||
|
|
||
| class ServiceBackend(Backend): | ||
| input_version = 2 |
There was a problem hiding this comment.
How come this versioning here? Seems arbitrary, is it ever checked? Are there instructions on how or when it should be updated?
There was a problem hiding this comment.
Sorry. It was old cruft from when I thought I had to make the spec backwards compatible.
daniel-goldstein
left a comment
daniel-goldstein
left a comment
There was a problem hiding this comment.
Sorry one more thing I noticed.
| from ..helpers import skip_unless_service_backend | ||
| from hail.backend.service_backend import ServiceBackend | ||
|
|
||
| CLOUD = os.environ['HAIL_CLOUD'] |
There was a problem hiding this comment.
Does this impede running other query tests locally? Not sure if it does but it would be annoying to have to specify HAIL_CLOUD to run tests that have nothing to do with it. I'd prefer in that case to put the os.environ call in the method that actually needs it and remove the env variable from test steps that aren't testing the service backend.
| from ..helpers import skip_unless_service_backend | ||
| from hail.backend.service_backend import ServiceBackend | ||
|
|
||
| CLOUD = os.environ.get('HAIL_CLOUD') |
There was a problem hiding this comment.
This is going to treat None and Azure the same. Can we keep this as os.environ['HAIL_CLOUD'] but just put this line inside the relevant test?
| if regions_str is not None: | ||
| regions = regions_str.split(',') | ||
| else: | ||
| regions = bc.supported_regions() |
There was a problem hiding this comment.
The regions parameter below is Optional[List[str]], but from this else case it looks like we don't exercise the None option. I don't have a strong opinion about whether to do one or the other, but I think we should either make regions not Optional, or keep this None instead of using supported_regions.
There was a problem hiding this comment.
I think we should try and keep the same interface as hailtop.batch as much as possible for consistency reasons. This would mean we'd need to expose the ServiceBackend.supported_regions and the equivalent of ServiceBackend.ANY_REGION. I think they should be hl.supported_regions() that is implemented as a static method on the service backend when applicable and hl.ANY_REGION is just an object. Once we figure this out, then we can figure out what makes sense for the types further downstream.
There was a problem hiding this comment.
I think we should try and keep the same interface as hailtop.batch as much as possible for consistency reasons.
Totally agree with this, but I see the constructor of the ServiceBackend object and its implementation as private, so I don't see why we can't currently write the type as it's being used -- if it's never None let's use that knowledge to cut down on None checks that are currently dead code.
There was a problem hiding this comment.
I agree that the user-facing API should be as close to batch as possible, but the ServiceBackend for query is not a user-facing API. Users only use hl.init which select the backend to use.
| await write_str(infile, orjson.dumps(reference_config).decode('utf-8')) | ||
| await write_str(infile, str(self.worker_cores)) | ||
| await write_str(infile, str(self.worker_memory)) | ||
| await write_int(infile, len(self.regions)) |
There was a problem hiding this comment.
This breaks if self.regions is None, so would need to change if we allow this to be optional
| Some(regionsArrayBuffer.toArray) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
When would the service_backend ever send an empty list?
daniel-goldstein
left a comment
daniel-goldstein
left a comment
There was a problem hiding this comment.
Left further comments in the thread.
daniel-goldstein
left a comment
daniel-goldstein
left a comment
There was a problem hiding this comment.
Requesting changes so it's off my CI page. Looks fine there's just merge conflicts now.
|
I know you've got a lot on your plate, but it would be awesome if we could get this in tomorrow or early next week. Of all my remaining PRs, I think this one is highest priority and should be close to being mergeable. |
daniel-goldstein
left a comment
daniel-goldstein
left a comment
There was a problem hiding this comment.
Thanks for the reminder. This looks good
3 participants
CHANGELOG: Added the regions parameter to
hl.init()for specifying which regions Query on Batch jobs should run in.