-
Notifications
You must be signed in to change notification settings - Fork 728
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
Use boto3 instead of awscli to download code package #474
Conversation
No idea why the build failed
|
I launched the tests again. Hopefully, it should resolve now. The existing test suite doesn't exercise the files that the PR touches. |
@tkislan Have you tested this PR? I am running into this error - |
I only tested the generated command that downloads the file Can you please provide more context? |
metaflow/metaflow_environment.py
Outdated
"%s -c \"" % self._python() | ||
+ "import boto3; " | ||
+ "exec('try:\\n from urlparse import urlparse\\nexcept:\\n from urllib.parse import urlparse');" | ||
+ "parsed = urlparse('%s');" % s3_path |
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.
@tkislan This parsing can happen while we are generating the get_s3_download_command
locally rather than in the remote environment. That will save us precious entry point chars.
metaflow/metaflow_environment.py
Outdated
""" | ||
|
||
return ( | ||
"%s -c \"" % self._python() |
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.
I doubt \"
would work here given that we use shlex
downstream for parsing the entry point.
The issue is with incorrect quoting given that we use
|
@savingoyal Thx for the pointer to This is the new full command that it generates, and I was able to run it locally
I had to go for the |
@savingoyal is anything still blocking this? |
Closing, as nobody cares .. |
Updated PR from #389
Resolves #387