-
Notifications
You must be signed in to change notification settings - Fork 33
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
Replace Portalocker #370
Replace Portalocker #370
Conversation
0735974 is fully disabling Windows testing. Is it on purpose or is additional work planned to selectively exclude the backend OMERO.tables tests on Windows? |
All CI tests are now successful and the OMERO.tables are skipped on Windows. I assume this will be assessed in a round to functional testing. Trying to identify a few scenarios which be reviewed in a deployment with this PR included:
Coming back briefly to the code changes and specifically a77749d, ome/openmicroscopy#3431 introduced a lot of logic specifically for handling OMERO.tables on Windows. Is it worth looking into reverting these changes as part of this clean-up to simplify our future maintenance? Or limit this to the minimal amount of changes and capture as an issue? |
I would rather keep this PR as simple as possible i.e. remove dependency so we can use Python 3.8+ |
👍 for the change in a post-Windows world but also for cleaning up the changes (in the future). |
issue created #373 Now that CI is back @pwalczysko @will-moore could you go over the scenarios highlighted above i.e.
|
OK - I remember we had issues with async parallel reading of tables before - see ome/omero-web#25 |
We still use |
I'm going to create a Table on merge-ci to test, but I'm not really sure what pass/fail looks like? |
Testing with Josh's script at https://forum.image.sc/t/server-error-after-populating-metadata-for-images-using-web-script/27844/14 gives the same results on merge-ci and on latest-ci (but not sure that even tests this PR)? |
As part of a general clean-up, it will be good to integrate this PR in the next release |
@jburel - thinking about a release now, should we get this merged? |
I think we can. |
Have all the scenarios mentioned in #370 (comment) been validated? It's not obvious from the following reviews in this PR |
It's not clear to me how to test concurrent access to OMERO config or OMERO.tables and what does a pass or fail look like? We probably need a server without portalocker enabled and a clear idea of how to demonstrate something bad happening (file corruption?) if we want to be sure that this is prevented by portalocker. |
For OMERO config, a minimal test is to call With the current OMERO.py release, I get
OMERO.tables will likely need a custom script testing something a similar scenario where a table is being locked and accessed at the same time by another process. |
With this branch installed locally, this works - (running this in 2 terminals at the same time):
Going on the tests as a guide, tried this...
Also tried this, which didn't throw an Exception, but not sure if this should?
Also just realised that this is testing server-side code, whereas I only have this PR installed client-side. |
Any more testing needed?
|
You're correct that the After a quick investigation, the most relevant place I found to test this logic are the OMERO.tables unit tests which have been de-activated since the Python 3 migration in b185956. jburel/omero-py@portalocker...sbesson:omero-py:table_locking_tests proposes 2 commits to re-activate these tests on top of this branch. d677199 in particular tests the |
@sbesson do you want to push the commits to this branch? |
Done. I believe the readthedocs CI build failure is unrelated |
It is |
As of the migration to Python 3 and f8af68d, both int and long values are returned as rlong
- Expect tables.open_file to open the file in append mode - Remove unnecessary print and FILE_OPEN_POLICY statement - Remove conditional handling of Pytables 2.x openFile API - Remove Pytables ValueError check (possibly invalid since Pytable 3.1)
I was trying to see where those unit tests are actually run, but I don't see anything explicit in the jobs above? |
sbesson@1ab20f7 is one way to address this issue and use the verbose flag to list skipped/passing/failing tests individually. Should I push it to this branch? |
@sbesson please push |
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.
In the logs for last build at https://github.com/ome/omero-py/actions/runs/6023964482/job/16341736551?pr=370
I see...
[gw2] PASSED test/unit/tablestest/test_hdfstorage.py::TestHdfStorage::testLocking
So I think we are good.
Replace internal portalocker by the upstream version. This is the first step to be able to upgrade to Python 3.8+ (windows)