- added windows support for pushing images to aws ecr #2500
Conversation
mparkhe
added
rn/breaking-change
|
@mparkhe Do i have to do something with this PR in order to move it forward? Or do i just wait? |
Hi @AndreyBulezyuk: We will take a look at this PR shortly. Thank you for your patience. |
mparkhe
requested review from
tomasatdatabricks and
dbczumar
and removed request for
tomasatdatabricks
There was a problem hiding this comment.
Hi @AndreyBulezyuk, thanks for the PR!
Looks good, I tested it manually on linux and mac and it works as expected.
I only had one small question.
| @@ -127,11 +128,23 @@ def push_image_to_ecr(image=DEFAULT_IMAGE_NAME): | |||
| # x = ecr_client.get_authorization_token()['authorizationData'][0] | |||
| # docker_login_cmd = "docker login -u AWS -p {token} {url}".format(token=x['authorizationToken'] | |||
| # ,url=x['proxyEndpoint']) | |||
| docker_login_cmd = "$(aws ecr get-login --no-include-email)" | |||
| os_command_separator = ";\n" | |||
| if platform.system() == "Windows": | |||
There was a problem hiding this comment.
It looks like we use sys.platform == "win32" elsewhere in the code to determine whether we're on Windows. Do you have a reason why do you use platform.system() == "Windows" instead? If it does not matter, we should use the same test. If it's more robust, we should replace the test in other places as a subsequent PR.
There was a problem hiding this comment.
I have no explicit reason for that, just made most sense. I also saw elsewhere in the code (project/init.py) that we use if os.name != "nt": to determine whether we are on windows.
I think i should research a bit more and find a more scalable way to figure out on what OS we are. Maybe i should take a look at how tensorflow or other major python projects do that?
Maybe an Object or Config File that would contain all resources for every os, like the terminal type, command separator, etc. Or maybe a library that does that.
There was a problem hiding this comment.
Seems like tensorflow uses 'platform' quite often. Here their a simple method for windows identification:
def _is_windows():
return platform.system() == "Windows"
Not sure if mlflow has a global repository of methods/helpers, but we could code a few OS identifying methods (like the one above) and use these helpers everywhere else in the code. This way if we decide to change the way we identify a OS, we must do so only in one location (helper/global methods).
How does that sound?
There was a problem hiding this comment.
Sounds great! Thanks for looking into it. I think we can do that in a followup though and merge this PR as is.
There was a problem hiding this comment.
Awesome, merging now, thanks again for the contribution!
There was a problem hiding this comment.
Are you in mlflow Slack? @tomasatdatabricks
There was a problem hiding this comment.
Yes I am (name is Tomas Nykodym).
* - added windows support for pushing images to aws ecr - import platform * - fixed linting issue * changed string format to python2 compatibility * minor string format correction * logging formatting interpolation
What changes are proposed in this pull request?
Bugfix 2311. Added support for Windows in the Sagemaker
mlflow sagemaker build-and-push-containerCLI Command.How is this patch tested?
Tested on Windows 10, Python 3.6.8 with execution of the code and successful deployment of local ML Model to AWS Sagemaker Service.
A final test on Linux machine would be great. (Can't do that at the moment)
Release Notes
Is this a user-facing change?
(MLFlow Sagemaker CLI now supports sagemaker deployment from a Windows machine.)
What component(s) does this PR affect?
How should the PR be classified in the release notes? Choose one:
rn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notes