Enhance cross-platform compatibility for loading PySRRegressor models
This PR improves the cross-platform compatibility of the PySRRegressor.from_file method, allowing seamless loading of models saved on different operating systems (Windows and Linux).
Key changes:
- Introduced a custom unpickler (
CustomUnpickler) to handle path objects from different operating systems. - Added a
path_to_strfunction to convert path objects to strings, ensuring consistent path handling across platforms. - Updated the model loading process to use the custom unpickler and path conversion.
- Maintained the existing functionality for creating models from scratch when a pickle file is not found.
These changes allow users to load PySRRegressor models on any supported platform, regardless of where the model was originally saved. This enhancement is particularly useful for users working in mixed environments or collaborating across different operating systems.
The modifications are contained within the from_file method to minimize impact on other parts of the codebase. The method remains backwards compatible with existing usage patterns.
Testing:
- Tested loading models saved on Linux from a Windows environment.
- Verified that the existing functionality for creating models from scratch remains intact.
This enhancement addresses the issue of cross-platform incompatibility when loading saved models, improving the overall user experience and flexibility of PySRRegressor.
Pull Request Test Coverage Report for Build 10138721258
Details
- 15 of 17 (88.24%) changed or added relevant lines in 2 files are covered.
- 3 unchanged lines in 1 file lost coverage.
- Overall coverage decreased (-0.3%) to 93.448%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| pysr/utils.py | 12 | 14 | 85.71% |
| <!-- | Total: | 15 | 17 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| pysr/export_sympy.py | 3 | 77.27% |
| <!-- | Total: | 3 |
| Totals | |
|---|---|
| Change from base Build 10137135457: | -0.3% |
| Covered Lines: | 1141 |
| Relevant Lines: | 1221 |
💛 - Coveralls
Thanks, this is great! I'll do a code review for suggesting some changes
(By the way, feel free to click "Resolved" on any of the comments once they are completed)
(By the way, feel free to click "Resolved" on any of the comments once they are completed)
Got it :)
Looking good! Thanks. I think it just needs some tests now – ideally tests that fail with the current master branch, but are fixed by this PR
One option is to add a test that is only run on the Windows tests – it could generate and then pickle the model on Windows, and then create and run a dockerfile that attempts to unpickle the model? (Since docker runs linux).
Looking good! Thanks. I think it just needs some tests now – ideally tests that fail with the current master branch, but are fixed by this PR
Thank you for the feedback. I agree tests would be valuable, but I'm not experienced in writing test cases. Could you provide some guidance or examples for appropriate tests for these changes?
One option is to add a test that is only run on the Windows tests – it could generate and then pickle the model on Windows, and then create and run a dockerfile that attempts to unpickle the model? (Since docker runs linux).
I appreciate your suggestion, but I'm afraid I'm a bit lost on how to implement this. The process of generating, pickling, and then using Docker for unpickling sounds complex. I'm not sure where to start. Would you be able to help implement this test, or provide a more detailed guide?
Ok what I would do is:
- Copy https://github.com/MilesCranmer/PySR/blob/master/pysr/test/test_dev.py to
pysr/test/test_pickle.py - Copy https://github.com/MilesCranmer/PySR/blob/master/pysr/test/test_dev_pysr.dockerfile to
pysr/test/test_pickle.dockerfile - Modify the code https://github.com/MilesCranmer/PySR/blob/3aee19e38ceb3e0e1617d357a831400e01204658/pysr/test/test_dev.py#L8-L47 within
test_pickle.pyto run a simple PySR example where the equation file parameter set to somePathto a folder created with https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory. Also change the class name toTestPickleor something. - Modify https://github.com/MilesCranmer/PySR/blob/3aee19e38ceb3e0e1617d357a831400e01204658/pysr/test/test_dev.py#L51-L59 to reference
TestPickle. - Search for any appearances of
test_devthroughout the codebase, and use that to figure out where you need to add the equivalenttest_pickle. - Modify the
test_pickle.dockerfile, specifically all of these lines: https://github.com/MilesCranmer/PySR/blob/3aee19e38ceb3e0e1617d357a831400e01204658/pysr/test/test_dev_pysr.dockerfile#L34-L55 to try to load the pickle file generated by the test you created in (3). And basically just verify that the equations loaded are correct. - Note how the code here: https://github.com/MilesCranmer/PySR/blob/3aee19e38ceb3e0e1617d357a831400e01204658/pysr/test/test_dev.py#L12-L47 builds and runs a dockerfile. Do the same in your new
test_pickle.py, but reference thetest_pickle.dockerfile. You should be able to pass the filename as an argument.
I think this should work. It might be overcomplicated though, maybe there's an easier way to test this. Feel free to take a different approach to test your new addition. For simple pure-Python tests you can just add a new test case to the class in test.py, such as https://github.com/MilesCranmer/PySR/blob/3aee19e38ceb3e0e1617d357a831400e01204658/pysr/test/test.py#L285-L297