(torchx/aws_batch) Enable Region Selection in TorchConfig #673
Conversation
|
Hi @ashvinnihalani! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
ashvinnihalani
force-pushed
the
awsbatch_region
branch
from
ee9ba8b
to
4c4ecfc
Compare
|
can you make sure the lint, pyre, and python unittests pass? |
| import boto3.session | ||
|
|
||
| return boto3.session.Session() | ||
| if region: | ||
| return boto3.session.Session(region_name=region).region_name |
There was a problem hiding this comment.
what is the point of providing a region then getting the region back from a boto3 session object?
There was a problem hiding this comment.
The original point was to check if a region name had been cached or specified in the config file or environment variable which is handled by the
return boto3.session.Session().region_name in line 260.
Adding a region parameter for the method signature similar to _local_session.
We can either remove the region parameter altogether or just return the region string without constructing a boto3.session object.
Personally, I am in favor of just returning the region string, that way if the priority of the region specification changes in the future we can just change the implementation without breaking backward compat.
| else: | ||
| return _local_session().client("batch") | ||
|
|
||
| def _regional_client(self, aws_region: Optional[str] = None) -> "boto3.session.Session": |
There was a problem hiding this comment.
no need to change the method name to _regional_client() since you added a region param. Lets keep the name as is.
| for qn in queue_names: | ||
| all_apps.extend(self._list_by_queue(qn)) | ||
| regions = _local_sessions_available_regions() | ||
| for region in regions: |
There was a problem hiding this comment.
have you measured how long this takes? last I tried this, it drastically worsens latency for m5 since we have a ton of job queues across regions. (by drastic I mean ~40seconds for the list API)
There was a problem hiding this comment.
Ill double-check for M5. However, did this even work across regions originally? A more apt comparison might be specifying the region in the .torchxconfig vs just using the other methods which I think I originally did.
QOL improvements for support for AWS regions.
Current Behavior: Use the default Region specified in .aws config
Proposed Behaviour: Have manual override in .torchxconfig. If no override and no default throw and error
Future Improvements: Add profile support. Add fargate support.