-
Notifications
You must be signed in to change notification settings - Fork 239
Fix FileNotFoundError in monitors and importers using improper NamedTemporaryFile pattern #7148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… for NamedTemporaryFile Co-authored-by: khsrali <26275340+khsrali@users.noreply.github.com>
Co-authored-by: khsrali <26275340+khsrali@users.noreply.github.com>
Co-authored-by: khsrali <26275340+khsrali@users.noreply.github.com>
Co-authored-by: khsrali <26275340+khsrali@users.noreply.github.com>
Co-authored-by: khsrali <26275340+khsrali@users.noreply.github.com>
27802b1 to
402d057
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7148 +/- ##
===========================================
+ Coverage 29.18% 79.61% +50.44%
===========================================
Files 566 566
Lines 43530 43590 +60
===========================================
+ Hits 12702 34702 +22000
+ Misses 30828 8888 -21940 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
khsrali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot changes looks good. Please try writing minimal tests
Co-authored-by: khsrali <26275340+khsrali@users.noreply.github.com>
Added minimal tests in commit cac5f4d:
Both tests use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes a FileNotFoundError that occurred when using NamedTemporaryFile('w+') with transport.getfile() operations. The open file handle conflicted with external file access, particularly on Windows and certain filesystems. The solution replaces the problematic pattern with tempfile.mkstemp() followed by explicit file descriptor closure and manual cleanup.
Key Changes:
- Replaced
NamedTemporaryFile(delete=True)pattern withmkstemp()+ manual cleanup in monitors and importers - Added test coverage to verify temporary file cleanup behavior
- Ensured pattern consistency with existing code in
cmd_computer.py
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/aiida/calculations/monitors/base.py |
Updated always_kill() to use mkstemp pattern for retrieving submission script via transport |
src/aiida/calculations/importers/arithmetic/add.py |
Updated parse_remote_data() to use mkstemp pattern for retrieving input file via transport |
tests/engine/processes/calcjobs/test_monitors.py |
Added test_always_kill_tempfile_cleanup() to verify proper cleanup of temporary files |
tests/calculations/importers/arithmetic/test_add.py |
Added test_parse_remote_data_tempfile_cleanup() to verify proper cleanup of temporary files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| finally: | ||
| try: | ||
| os.remove(temp_path) | ||
| except OSError: |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'except' clause does nothing but pass and there is no explanatory comment.
The
always_killmonitor andArithmeticAddCalculationImporterwere causingFileNotFoundErrorwhentransport.getfile()attempted to write to temporary files created withNamedTemporaryFile('w+'). The open file handle conflicted with external file access, particularly on Windows and certain filesystems.Changes
NamedTemporaryFile(delete=True)withtempfile.mkstemp()in monitor and importer codecmd_computer.pyfor transport operationsfinallyblock with OSError handlingtest_always_kill_tempfile_cleanup: Verifies the monitor properly cleans up temporary filestest_parse_remote_data_tempfile_cleanup: Verifies the importer properly cleans up temporary filesBefore:
After:
Files Modified
src/aiida/calculations/monitors/base.py-always_kill()functionsrc/aiida/calculations/importers/arithmetic/add.py-ArithmeticAddCalculationImporter.parse_remote_data()methodtests/engine/processes/calcjobs/test_monitors.py- Added cleanup test foralways_kill()tests/calculations/importers/arithmetic/test_add.py- Added cleanup test forparse_remote_data()Original prompt
This section details on the original issue you should resolve
<issue_title>🐛
FileNotFoundErrorrelated to monitors</issue_title><issue_description>While "running" a workflow, I am reliably running into the following error:
From the full traceback (see below), the issue seems to be related to the monitors. In the end, the workflow does complete successfully, but the issue produces a lot of noise that will worry users.
I tested both
core.sshandcore.ssh_async, and am running into the issue for both.Full Traceback
Comments on the Issue (you ar...
FileNotFoundErrorrelated to monitors #7086✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.