Rs/support rm progress #1664
Rs/support rm progress #1664
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1664 +/- ##
==========================================
+ Coverage 76.78% 76.83% +0.04%
==========================================
Files 59 59
Lines 8733 8763 +30
Branches 1424 1427 +3
==========================================
+ Hits 6706 6733 +27
- Misses 1829 1831 +2
- Partials 198 199 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
romasku
force-pushed
the
rs/support-rm-progress
branch
2 times, most recently
from
6b12bde to
1f1b0f8
Compare
|
Uhh, the PR is quite longer than I expected. |
Sorry for that. I tried to do work step by step in commits. If you want, I can close this PR and create smaller ones for different commits. |
neuromation/api/storage.py
Outdated
| @@ -735,67 +785,23 @@ def _file_status_from_api_stat(cluster_name: str, values: Dict[str, Any]) -> Fil | |||
| ProgressQueueItem = Optional[Tuple[Callable[[Any], None], Any]] | |||
|
|
|||
|
|
|||
| class QueuedProgress: | |||
There was a problem hiding this comment.
Why it was necessary to rewrite the code related to progress?
There was a problem hiding this comment.
QueuedProgress was already little bit wrongly typed: it was used for both AbstractFileProgress and AbstractRecursiveFileProgress with casts like the following:
progress = cast(AbstractRecursiveFileProgress, self._progress)
Moreover, AbstractDeleteProgress is not subclass of AbstractFileProgress, so trying to reuse it would lead to even more strange typing. So I decided to generalize the pattern (put calls into asyncio.Queue) to make it easy to reuse it for my case and make for solid typing by adding _AsyncAbstract... classes. Initially I thought that I will be able to do proper annotations using generics, but I don't know any way to say mypy that function returns almost same type but all methods are replaced with coroutines.
There was a problem hiding this comment.
Then I think that it would be better to split this PR on two parts. One implements the rm progress and other refactors the progress machinery.
There was a problem hiding this comment.
PR for refactoring the progress machinery: #1667
Sorry that I didn't do this initially.
neuromation/api/utils.py
Outdated
| @@ -127,3 +129,63 @@ def find_project_root(path: Optional[Path] = None) -> Path: | |||
| return here | |||
| here = here.parent | |||
| raise ConfigError(f"Project root is not found for {path}") | |||
|
|
|||
|
|
|||
| class QueuedCall: | |||
There was a problem hiding this comment.
Why not use just partial()?
|
@romasku no, the PR size is acceptable. |
romasku
force-pushed
the
rs/support-rm-progress
branch
from
1f1b0f8 to
6ed9521
Compare
CHANGELOG.D/1664.feature
Outdated
| @@ -0,0 +1 @@ | |||
| Added `progress` argument to `Storage.rm` for tracking delete progress | |||
There was a problem hiding this comment.
Please document also the CLI option. Most users use the CLI.
neuromation/api/storage.py
Outdated
| async with self._core.request("DELETE", url, auth=auth) as resp: | ||
| resp # resp.status == 204 | ||
| def server_status_to_uri(path: str) -> URL: | ||
| base_uri = URL.build(scheme="storage", authority=self._config.cluster_name) |
There was a problem hiding this comment.
It is constant and can be created outside of the loop.
Closes #1663