-
Notifications
You must be signed in to change notification settings - Fork 77
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
Ensure XML files are opened with UTF-8 encoding #580
Conversation
I'm trying to rerun the tests, but they keep getting skipped.. If I run the tests locally I'm getting an error: ============================================= ERRORS =============================================
__________________________ ERROR collecting foyer/tests/test_trappe.py ___________________________
tests/test_trappe.py:13: in <module>
TRAPPE_UA = Forcefield(name="trappe-ua")
forcefield.py:522: in __init__
preprocessed_files = preprocess_forcefield_files(all_files_to_load)
forcefield.py:80: in preprocess_forcefield_files
xml_contents = f.read()
E ValueError: I/O operation on closed file.
======================================== warnings summary ========================================
../../../../miniconda3/envs/foyer-dev/lib/python3.12/site-packages/mdtraj/formats/__init__.py:13
/home/chris/miniconda3/envs/foyer-dev/lib/python3.12/site-packages/mdtraj/formats/__init__.py:13: DeprecationWarning: 'xdrlib' is deprecated and slated for removal in Python 3.13
from mdtraj.formats.trr import TRRTrajectoryFile
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
---------- generated Nunit xml file: /home/chris/cme/repos/foyer/foyer/test-output.xml -----------
==================================== short test summary info =====================================
ERROR tests/test_trappe.py - ValueError: I/O operation on closed file. It looks like calling 67 for xml_file in forcefield_files:
68 if not hasattr(xml_file, "read"):
69 try:
70 f = open(xml_file, encoding="utf-8")
71 finally:
72 f.close()
73
74 _, suffix = os.path.split(xml_file)
75 else:
76 f = xml_file
77 suffix = ""
78
79 # read and preprocess
80 xml_contents = f.read()
81 f.close() |
Strange to see .NET stuff float in!
|
8041202
to
2707210
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #580 +/- ##
==========================================
+ Coverage 62.01% 62.04% +0.02%
==========================================
Files 16 16
Lines 1664 1665 +1
==========================================
+ Hits 1032 1033 +1
Misses 632 632 |
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.
LGTM, thanks!
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.
Ran tests locally on my end as well here and it looks good, so I'm going to merge this one.
Thanks @CalCraven and @chrisjonesBSU! |
PR Summary:
This PR ensures XML files are opened in UTF-8 mode while being processed. The background here is ... messy and hard to reproduce. What I'm fairly sure of is that my testing infrastructure writes some temporary files (including Foyer XML files) and something might be loaded under an assumption of ASCII encoding. This causes a crash similar to forcing that behavior:
This is because of the fancy (unicode)$\alpha$ here:
AFAICT, Python 3 defaults to
open()
ing files in UTF-8 mode, so this change shouldn't impact anything but esoteric cases like I've run into. (Handwaving more, it's a fixture that crashes only at collection time and only when usingpytest-xdist
.) This would be tricky for me to write a unit test for, but I can take a stab at it if desired.PR Checklist