Skip to content
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

[Discussion] API for progress report #687

Closed
asvetlov opened this issue Apr 4, 2019 · 2 comments
Closed

[Discussion] API for progress report #687

asvetlov opened this issue Apr 4, 2019 · 2 comments
Labels

Comments

@asvetlov
Copy link
Contributor

asvetlov commented Apr 4, 2019

The issue is a site for discussion a possible public API improvement.
Please think about using neuromation as a public library, not CLI tool only.

Problem

Now we have await client.storage.upload_file(progress, src_url, dst_url) for uploading file. The same is for downloading, working with directories and images.

The current solution is not good: many times people don't need a progress report but want to just upload a file, waiting for the upload finishing. `

Solution 1

The obvious change is making progress an optional argument. If the progress report is not needed await client.storage.upload_file(src_url, dst_url) can be used. For getting progress info there is an await client.storage.upload_file(src_url, dst_url, progress=progress) call variant.

It works but implementing an AbstractProgress class and using it in client code is a cumbersome task.

Solution 2

Another option is getting rid of AbstractProgress at all.

Simple form remains the same, progress report is received as async iterator:

async for progress in client.storage.upload_file(src_url, dst_url):
    if type(progress) == FileStart:
         update_start(progress.local_url, progress.remote_url)
    elif type(progress) == FileComplete:
         update_complete(progress.local_url, progress.remote_url)
    elif type(progress) == FileProgress:
         update_progress(progress.local_url, progress.remote_url, progress.bytes_sent, progress.file_size)

Implementation of upload_file can return an object which is both awaitable and async iterable.
Simple form is used when await obj is called (__await__ dunder method is used).
Progress report is sent if the object is used as async iterator (__aiter__ is called).
Either __await__ or __aiter__ is allowed, not both. Not calling await / async for produces RuntimeError on garbage collection.

Comparison

Both approaches have a similar functionality, both are forward-compatible (new progress states can be added without breaking existing code).
I'm inclining to solution 2 but have a doubt if the proposal usage is clean and obvious enough to newbies.
I never seen proposed magic usage before.
On other hand it is not hard to use, it aiohttp we have something similar for optional context manager protocol for 4 years, nobody complains.

Please share your opinion.

The issue is not urgent but we need to solve it somehow before making our API public.

@astaff astaff added the platform label Apr 4, 2019
@shagren
Copy link
Contributor

shagren commented Apr 4, 2019

I think neuro is(must be?) best gui tool for platform interaction.
Most users will use API for some automation and will ignore progress reporting often. Then first solution with optional progress will be better.

@asvetlov
Copy link
Contributor Author

asvetlov commented Apr 4, 2019

Most users don't need progress report when working with API, true.
But we need a convenient API for the rest.
The second solution supports await client.storage.upload_file(progress, src_url, dst_url) as well, BTW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants