fix: resolved Windows file permission error#3269
Conversation
Greptile SummaryThis PR fixes a Windows
Confidence Score: 4/5The change is safe to merge; the core tempfile-close-before-rename fix is correct and the other improvements are low-risk. The functional changes (close before rename, os.replace, fsync, None guard) are all correct. The only concerns are two Python 2 compatibility shims that are unreachable dead code and a redundant close() call in the except block — none of these affect runtime behaviour on any supported platform. metaflow/client/filecache.py — the Python 2 shims (urlparse fallback and _replace wrapper) should be removed before merging to keep the file clean. Important Files Changed
Reviews (1): Last reviewed commit: "made replace python 2 compatible" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| # Python 3 | ||
| from urllib.parse import urlparse | ||
| except ImportError: | ||
| # Python 2 | ||
| from urlparse import urlparse |
There was a problem hiding this comment.
urlparse fallback unreachable
Metaflow requires Python 3.6+, so the except ImportError branch importing from urlparse import urlparse can never execute. The original single-line import was correct. This try/except adds noise without benefit.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
The duplicate tmpfile.close() is required. If an i/p exception occurs during write(), flush(), or fsync(), the file is never closed in the try block.
There was a problem hiding this comment.
Your comment seems to be addressing a different change than what my comment was about — I flagged the urlparse try/except as dead code (since Metaflow requires Python 3.6+), while you're discussing the tmpfile.close() in the except block of create_file.
On the tmpfile.close() point: you're correct. If write(), flush(), or fsync() raises, the tmpfile.close() inside the try block is never reached, so the handle would leak without the one in except. That's a valid and necessary pattern.
My original comment stands separately — the urlparse try/except wrapping from urllib.parse import urlparse is still unreachable dead code given Metaflow's Python 3.6+ requirement, and the original single-line import was fine.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| if sys.version_info[0] >= 3: | ||
| _replace = os.replace | ||
| else: | ||
| import errno | ||
|
|
||
| def _replace(src, dst): | ||
| try: | ||
| os.rename(src, dst) | ||
| except OSError as e: | ||
| if e.errno == errno.EEXIST or (os.name == "nt" and e.errno in (13, 17)): | ||
| os.remove(dst) | ||
| os.rename(src, dst) | ||
| else: | ||
| raise |
There was a problem hiding this comment.
_replace fallback is dead code
The else branch (Python < 3) and the custom _replace function are unreachable given Metaflow's Python 3.6+ requirement. os.replace (added in Python 3.3) is always available, handles atomic overwrite on all platforms including Windows, and can be called directly. The sys.version_info guard and the fallback implementation just add complexity. Consider replacing all uses of _replace with os.replace directly.
| except Exception: # noqa E722 | ||
| tmpfile.close() | ||
| if os.path.exists(tmpfile.name): | ||
| os.unlink(tmpfile.name) |
There was a problem hiding this comment.
close() call in the exception handler
tmpfile.close() is already called in the try block (line 296) before _replace. If _replace raises, the except block calls tmpfile.close() again on an already-closed file. While this is safe (Python's file close() is idempotent), it may be clearer to restructure with finally or remove the duplicate call.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@talsperre, could you confirm whether Metaflow is still compatible with Python 2? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3269 +/- ##
=========================================
Coverage ? 29.08%
=========================================
Files ? 381
Lines ? 52532
Branches ? 9270
=========================================
Hits ? 15278
Misses ? 36217
Partials ? 1037 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR Type
Summary
Replaced os.rename with os.replace, and ensured temporary files are explicitly closed and synced to disk before moving. This prevents PermissionErrors caused by file locks on Windows.
Added a None check in
get_log_legacyto prevent the datastore from attempting to write empty log files to the cache.Swapped an O(N) list .insert() with an O(1) .append() in
_index_objects()to speed up lazy indexing of cache objects. Because at the end that was sorted so the indexing does not matters.Added python 2 compatible import and
_replacefunction.