-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Feature/aws sagemaker instance update #8880
Feature/aws sagemaker instance update #8880
Conversation
Hi there ! Is there any thing block this PR ? We will be happy to use |
@ltupin Not to my knowledge, just waiting for it to be reviewed. |
Dear reviewers team could you please check and review this PR :-D ? |
@kylebechtel77 there are still a few comments and TODOs in the code that I left in my old PR. Do you want me to work on those? I think, I can find some time next week to finish them. |
@jckuester That would be great! |
@bflad I know you were watching @jckuester PR on this before, is there anything else we need to do to get this merged? I know @jckuester has some todo's he'd like to take care of, but that can be a separate PR, myself and @ltupin need the functionality thats already in here sooner rather than later. |
+1 on getting this reviewed and merged |
hey @kylebechtel77 , do you mind rebasing this? |
Could we get this reviewed and merged? Some of the projects I'm working on will need the extra options that are included here. |
Hi @bflad, I once started this PR and would like to help to finish what I started: As the scope "sagemaker instance update" is probably too broad, would it help you if I split this PR up into smaller PRs for one attribute each (e.g., one for |
Hey @jckuester, I think it would be best to split the update functionally to existing arguments and open separate PRs for new arguments (there are also a few existing ones for some of the ones added here but the do not handle update) |
Closing this in favour of #15385 that been merged. thanks for all your work on this! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
This is just an updated PR based on this PR by @jckuester
#8011
Merge conflicts have been fixed and I will try to keep up with any feedback made on this PR.
I'm fairly new at this so let me know if I did anything incorrectly here, thanks!
Changes proposed in this pull request: