fix: Trial APIs --local should respect .detignore. [MLG-1352]
#8683
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8683 +/- ##
==========================================
+ Coverage 51.46% 51.48% +0.01%
==========================================
Files 884 886 +2
Lines 156121 156170 +49
Branches 2088 2088
==========================================
+ Hits 80352 80397 +45
- Misses 74282 74286 +4
Partials 1487 1487
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ioga
changed the title
--local should respect .detignore. [DET-1352]--local should respect .detignore. [MLG-1352]
| # We could use pathlib.Path.rglob for scanning the directory; | ||
| # however, the Python documentation claims a warning that rglob may be | ||
| # inefficient on large directory trees, so we use the older os.walk(). |
There was a problem hiding this comment.
This comment is ancient, but actually I think the fact that we skip directories is a much better reason to us os.walk.
I recommend just deleting the comment.
harness/determined/util.py
Outdated
| if on_cluster: | ||
| model_dir = constants.MANAGED_TRAINING_MODEL_COPY | ||
| ignore_func = shutil.ignore_patterns("__pycache__") |
There was a problem hiding this comment.
Delete this line.
When we are on-cluster, we get the model definition from a special clean copy of the model definition, which can't have __pycache__ because __pycache__ is part of DEFAULT_DETIGNORE.
This line of code traces back to f14f769 where the src arg to copytree() was os.getcwd(). I don't think it's relevant anymore, and I think it's safe to delete.
| try: | ||
| entry = v1file_utils.v1File_from_local_file(entry_path, file_path) | ||
| except OSError: | ||
| print(f"Error reading '{entry_path}', skipping this file.") |
There was a problem hiding this comment.
can you set file=sys.stderr on this print statement?
| file_path = pathlib.Path(path) / name | ||
| file_rel_path = file_path.relative_to(root_path) | ||
|
|
||
| if ( | ||
| file_path.is_dir() and ignore_spec.match_file(str(file_rel_path) + "/") | ||
| ) or ignore_spec.match_file(str(file_rel_path)): |
There was a problem hiding this comment.
nit: why bounce back and forth between pathlib.Path and str? Why not just use os.path.join() and os.path.relpath()?
non-blocking; feel free to ignore my inner C programmer which cringes at unnecessary malloc() calls.
Description
When trial APIs in
--localmode are making checkpoints, they should respect.detignoresimilar to the usual context directory creation.Test Plan
--localrun.For example,
det e create ... --local -test— initial state, works oktouch root.txtsudo chown root root.txtsudo chmod 400 root.txtdet e create ... --local -test— will fail because of a permission errorecho root.txt >> .detignoredet e create ... --local -test— now works okCommentary (optional)
Checklist
docs/release-notes/.See Release Note for details.
Ticket