Fix syncing offline runs with file upload #1211
Conversation
Codecov ReportBase: 79.55% // Head: 71.51% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1211 +/- ##
==========================================
- Coverage 79.55% 71.51% -8.04%
==========================================
Files 288 288
Lines 13624 13641 +17
==========================================
- Hits 10838 9756 -1082
- Misses 2786 3885 +1099
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -522,7 +526,7 @@ def _execute_upload_operations( | |||
| swagger_client=self.leaderboard_client, | |||
| container_id=container_id, | |||
| attribute=path_to_str(op.path), | |||
| source=op.file_path, | |||
| source=op.get_absolute_path(operation_storage), | |||
There was a problem hiding this comment.
Looks like both - clear and get_absolute_path should be defined in OperationStorage. Operation should be a simple serializable dataclass with minimum logic. And it would be better to pass operation object instead of raw path as an argument.
There was a problem hiding this comment.
Ok, I'll give it a try.
There was a problem hiding this comment.
But if we define clear and get_absolute_path in OperationStorage we'll have to pass Operation (in particular UploadFile) to those functions because we would like to clear or get path of particular operation.
At the same time we need upload_path of OperationStorage when creating operation (when we create UploadFile of in-memory file we also save file to .neptune catalog).
It'll require OperationStorage to be some kind of proxy on UploadFile operation irresponsible also for creating it.
If you're ok with that I can do that, what do you think?
| ) | ||
| tmp_file_name = cls.get_tmp_file_name(attribute_path, value.extension) | ||
| value._save(upload_path / tmp_file_name) | ||
| operation = UploadFile(path=attribute_path, ext=value.extension, tmp_file_name=tmp_file_name) |
There was a problem hiding this comment.
clean_after_upload is missing. Is it intentional?
There was a problem hiding this comment.
Yes. Now we determine if operation should be cleaned by existence of tmp_file_name value.
clean_after_upload is deprecated flag leaved only for handling of old format operations if user invoke sync on old data.
| file_path: str | ||
| file_path: str = None | ||
| tmp_file_name: str = None | ||
| # `clean_after_upload` is for backward compatibility and should be removed in the future | ||
| clean_after_upload: bool = False | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Maybe pass OperationStorage instead of upload_path?
shnela
force-pushed
the
jk/upload-from-offline
branch
from
2f1fe99
to
a59260d
Compare
No description provided.