-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add ability to run on an ec2 instance #272
Conversation
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.
Looking good! Just minor comments/questions
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #272 +/- ##
==========================================
+ Coverage 49.39% 50.04% +0.64%
==========================================
Files 19 19
Lines 1166 1201 +35
==========================================
+ Hits 576 601 +25
- Misses 590 600 +10 ☔ View full report in Codecov by Sentry. |
ImageId='ami-07d9b9ddc6cd8dd30', | ||
InstanceType='t2.medium', |
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.
Should these be configurable arguments on the function?
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.
This was one I was wondering about. This requires, the user to put in a lot more information about the configuration. The block mapping in line 548, those fields depend on the image. Instance type depends on the image. I think it's fine to hard code because these configurations exist and would require different scripts for each type of VM and errors on the ec2 instance (not enough memory, incorrect os script, etc.) are pretty much on the user to debug.
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.
Let's leave out configuration for now and add it later if we need to
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.
🚢
resolves #265
Add the field
run_on_ec2
which spins up an EC2 instance with the environment credentials, runs the latest version of sdgym with the same parameters and saves the output to the s3 bucket. Terminates the Ec2 instance once finished with the process to avoid keeping paid resources alive.Only issue is that custom synthesizers may not work with this command in this PR (as that requires a standardized way to bring in custom imported classes to the EC2 instance as well)