-
Notifications
You must be signed in to change notification settings - Fork 2
Update python setup logic #111
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 97.11% 97.09% -0.03%
==========================================
Files 8 8
Lines 243 241 -2
==========================================
- Hits 236 234 -2
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…or reload method and move tests for Zarr class into a separate file
zarrPy = py.importlib.import_module('ZarrPy'); | ||
end | ||
|
||
function pyReloadInProcess() |
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.
Where is this called?
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.
NM, I missed the tests. Is it only for the tests?
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.
This is for development convenience. If Python is in-process and we modify the Python code, this method can be used after clear classes
to reload the Python module to pick up the code changes.
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.
Ahhhh- you could do something like having a constant hidden property that cleans up and executes that function so clear classes
would do this automagically.
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.
I've discussed different approaches with external interface team and didn't arrive to anything much cleaner, so time-boxed it to this solution. But will follow up with you offline to see if we can improve it.
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.
Thanks for updating the tests.
I've consulted with the Python interface team and reworked the logic for python setup. Changes include:
py.importlib.reload
into a separate method,Zarr.pyReloadInProcess()
. Reloading of the python module is only needed during development - when the python code is changed and MATLAB is using in-process python. In that case, the developer needs to do the following in order for the updates in the python code to be picked up:If they are using out-of-process python, they can just do
terminate(pyenv)
Instead of using
py.ZarrPy
to refer to the Python module, updated the code to always use the return ofpy.importlib.import_module('ZarrPy')
. Usingpy.ZarrPy
should normally work, but there seems to be some issue with that workflow at the moment (especially with out-of-process python). So using the return ofpy.importlib.import_module('ZarrPy')
is currently more robust. We are ok to do multiple calls topy.importlib.import_module('ZarrPy')
without any performance penalty, as it internally caches the module after the first call.Added a test for the new
Zarr.pyReloadInProcess()
method. It is just a sanity check to make sure using the method does not break any functionality. Since we now have two tests for functions of Zarr class, created a separate tZarr file for those kinds of unit tests.