Add file datatype type to support saving and reading files/folders in artifact_store #1805
Conversation
jieguangzhou
changed the title
jieguangzhou
force-pushed
the
feat/save-directory
branch
from
77ec8c8
to
163caa8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1805 +/- ##
===========================================
- Coverage 80.33% 65.91% -14.43%
===========================================
Files 95 124 +29
Lines 6602 8842 +2240
===========================================
+ Hits 5304 5828 +524
- Misses 1298 3014 +1716 ☔ View full report in Codecov by Sentry. |
jieguangzhou
force-pushed
the
feat/save-directory
branch
5 times, most recently
from
7f7c213
to
78ebe53
Compare
jieguangzhou
force-pushed
the
feat/save-directory
branch
2 times, most recently
from
34609c5
to
54e0bd3
Compare
jieguangzhou
force-pushed
the
feat/save-directory
branch
from
54e0bd3
to
8708513
Compare
| save_path = os.path.join(file_id_folder, name) | ||
| logging.info(f"Copying file {file_path} to {save_path}") | ||
| if path.is_dir(): | ||
| shutil.copytree(file_path, save_path) |
There was a problem hiding this comment.
@jieguangzhou if there is a partially copy and program crashes may be a rollback?
possibly in future if not in this pr.
Ignore if already happens in shut.copy*
There was a problem hiding this comment.
Yes, we need to do this in future, not only filesystem, all the artifact store need to support this
There was a problem hiding this comment.
This should be handled in db.add. We need to be able to roll back the whole add process.
There was a problem hiding this comment.
- Deleting already added artifacts
- Cancelling any running jobs
- Removing computed outputs
Not an easy task.
| save_path = os.path.join(file_id_folder, name) | ||
| logging.info(f"Copying file {file_path} to {save_path}") | ||
| if path.is_dir(): | ||
| shutil.copytree(file_path, save_path) |
There was a problem hiding this comment.
@jieguangzhou @blythed
Since we are copying it in artifact as a snapshot of the directory, we need to caution users that if they change the source directory after db.load , it will not be reflected in the component.
although it is intended to be like this
| """Download file or folder from GridFS and return the path""" | ||
| temp_dir = tempfile.mkdtemp(prefix=file_id) | ||
|
|
||
| # try to download a file first, if it fails, assume it's a folder |
There was a problem hiding this comment.
this comment is not valid anymore right since you have a metadata.type with 'dir' or 'file' which will give you if folder or file.
There was a problem hiding this comment.
When downloading, we currently only have file_id information and no other information. Although we can also use file_id to judge first, it will increase a search.
Or save more information upstream, but actually increase the complexity
WDTY?
There was a problem hiding this comment.
It should be explicit. If we don't have the type then we should raise an Exception. Don't assume anything.
jieguangzhou
force-pushed
the
feat/save-directory
branch
3 times, most recently
from
c0fdf02
to
a065b06
Compare
jieguangzhou
force-pushed
the
feat/save-directory
branch
from
a065b06
to
61ced9a
Compare
Description
#1789
Related Issues
Checklist
make unit-testingandmake integration-testingsuccessfully?Additional Notes or Comments