Client-side support for streaming neuro-save #946
Conversation
neuromation/api/jobs.py
Outdated
| @@ -162,14 +171,43 @@ def __init__(self, core: _Core, config: _Config) -> None: | |||
| # an error is raised for status >= 400 | |||
| return None # 201 status code | |||
|
|
|||
| async def save(self, id: str, image: RemoteImage) -> None: | |||
| async def save( | |||
There was a problem hiding this comment.
Looking on dependency graph I'm inclining to a thought that this method actually belongs to images, not jobs.
There was a problem hiding this comment.
as far as I understood, the CLI command should be neuro job save, not neuro image save 😕
There was a problem hiding this comment.
This is a good question.
In docker save belongs to docker image group, not docker container.
Maybe we should apply the same change to our CLI.
Anyway, for now please keep CLI command as is but move API.
There was a problem hiding this comment.
I moved both save() back to jobs because when it belongs to images, this functionality does not work on windows as Images.__init__() requires docker available on the local machine, which is not necessary for save().
Let's keep it less clean for now, and later refactor it out to a separate class (a separate member of Client).
There was a problem hiding this comment.
Separate member of Client sounds even better.
What name do you suggest?
What prevents you from making this refactoring now?
There was a problem hiding this comment.
What name do you suggest
maybe something like ImagesLocal (for current Images) and ImagesRemote? meaning if the user's machine works with local docker or remote docker.
What prevents you from making this refactoring now?
It will require many non-functional changes; let's keep this PR as-is and do refactoring it in a separate PR?
atemate
force-pushed
the
ay/docker-stream-retry
branch
from
f1b29f8 to
def39d3
Compare
Codecov Report
@@ Coverage Diff @@
## master #946 +/- ##
==========================================
+ Coverage 87.3% 87.57% +0.27%
==========================================
Files 37 37
Lines 4215 4259 +44
Branches 636 641 +5
==========================================
+ Hits 3680 3730 +50
+ Misses 420 413 -7
- Partials 115 116 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #946 +/- ##
==========================================
+ Coverage 92.21% 92.47% +0.25%
==========================================
Files 37 37
Lines 4253 4345 +92
Branches 640 647 +7
==========================================
+ Hits 3922 4018 +96
+ Misses 223 220 -3
+ Partials 108 107 -1
Continue to review full report at Codecov.
|
| @@ -162,15 +157,6 @@ def __init__(self, core: _Core, config: _Config) -> None: | |||
| # an error is raised for status >= 400 | |||
| return None # 201 status code | |||
|
|
|||
| async def save(self, id: str, image: RemoteImage) -> None: | |||
There was a problem hiding this comment.
moved to api/images.py
atemate
changed the title
neuromation/api/abc.py
Outdated
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ImageCommitStep: |
There was a problem hiding this comment.
Looks like you have always two ImageCommitStep messages: one with status==CommitStarted, second with status=CommitFinished.
Why not replace it with two distinct message type:
@dataclass
class ImageCommitStarted:
pass # really no context data? I expect job id at least
@class
class ImageCommitFinished:
container: str
target_image: RemoteImage
# job id again, maybe?
There was a problem hiding this comment.
Honestly, I don't see the difference. Re-worked.
neuromation/api/abc.py
Outdated
| @dataclass(frozen=True) | ||
| class ImageCommitStarted: | ||
| job_id: str | ||
| container: str |
There was a problem hiding this comment.
What container string is?
In jobs API we have complex Container dataclass. I have an understanding of what dataclass fields mean. We'll have documentation for it soon.
As a user, I have no idea what to do with container-as-string except printing it may be (but what is the string format)?
There was a problem hiding this comment.
it's 64-long SHA256 hash of docker container
There was a problem hiding this comment.
mostly technical information, but since we reveal operations "commit" and "push", I decided to print it as well.
There was a problem hiding this comment.
Hash of docker container or image?
If this is a local docker container hash -- how can I use this info as a client?
Client has no access to docker daemon deployed on the node.
There was a problem hiding this comment.
hash of docker container.
Client has no access to docker daemon deployed on the node.
true. I will remove container: str.
There was a problem hiding this comment.
In this case, we also should not print Pushing image artemyushkovskiy/test:latest => image://artemyushkovskiy/test:latest because the source image does not make sense for the user
There was a problem hiding this comment.
I removed messages about "container", but I will not fix docker-push messages "Pushing image artemyushkovskiy/test:latest => image://artemyushkovskiy/test:latest" in this PR as it is a very minor issue that requires lots of changes.
|
This pull request fixes 5 alerts when merging 6831aaf into 9b5e44f - view on LGTM.com fixed alerts:
|
No description provided.