Skip to content
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

Removal of python-future compatibility code #390

Merged
merged 26 commits into from
Feb 22, 2024

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Jan 18, 2024

Fixes #389

Primarily motivated by the support of Python 3.12 in OMERO and given the current maintenance state of the python-future project, this reviews the OMERO.py source code to update all paths with either Python 2 or Python 2/3 logic to pure Python 3+.

  • all imports from the future dependency have been reviewed and cleaned up. The commits should contain some details but capturing a few specific notes on updates that might require particular attention during the review
    • isbytes and bytes_to_native_str have been replaced by isinstance(x, bytes) and x.decode('utf-8')
    • old_div has been replaced by / for path concatenation and either / or // for mathematical divisions depending on the use case
    • long has been removed as it is no longer a basic type in Python 3
    • basestring has been replaced by str and/or bytes depending on the use case
  • all imports builtins have been cleaned up as they should be unnecessary
  • all Python 2 codepaths using the value of sys.version_info have been cleaned up

This branch has been minimally tested in an Ubuntu 22.04 deployment (Python 3.10) with OMERO.server and OMERO.web. There are a few initial questions:

  • should the future dependency be fully removed from setup.py or kept for compatibility with the downstream projects?
  • considering team testing, several downstream Python projects including OMERO.web, openmicroscopy, various OMERO CLI plugins might need a similar handling. How many do we want to be patched in a similar fashion before scheduling a round of functional testing?

Initially opening as a draft PR to allow and gather feedback. I propose to get in the nightly CI builds over the week-end to get extra coverage from the integration tests and fix additional outstanding issues and we can discuss the progress next week.

@sbesson sbesson marked this pull request as ready for review January 19, 2024 21:16
@snoopycrimecop
Copy link
Member

snoopycrimecop commented Jan 20, 2024

Conflicting PR. Removed from build OMERO-python-superbuild-push#424. See the console output for more details.
Possible conflicts:

--conflicts Conflict resolved in build OMERO-python-superbuild-push#425. See the console output for more details.

@sbesson
Copy link
Member Author

sbesson commented Jan 23, 2024

Assigning a few people to start the code review. As discussed earlier today with @jburel, @joshmoore @pwalczysko @khaledk2 and @dominikl, the next proposed steps are:

  • open similar changes cleaning up future and the legacy Python 2 compatibility code against omero-dropbox & omero-web
  • work on a set of scenarios for the testing of= OMERO.web and OMERO.py
  • schedule a round of collective testing

@jburel
Copy link
Member

jburel commented Jan 26, 2024

Shouldn't block like

try:
    from StringIO import StringIO
    from StringIO import StringIO as BytesIO
except ImportError:
    # Python 3
    from io import StringIO
    from io import BytesIO

be adjusted too? (this is from setup.py)

@jburel
Copy link
Member

jburel commented Jan 26, 2024

'future' is still listed as a dependency even if its usage has been removed. Is that intended for potential extension/plugin?

@jburel
Copy link
Member

jburel commented Jan 26, 2024

The python_warning.py module should probably be removed too.

@jburel
Copy link
Member

jburel commented Jan 26, 2024

What about cases in omero_ext e.g.py_version = sys.version_info?

@sbesson
Copy link
Member Author

sbesson commented Jan 26, 2024

Thanks for the feebdback. Pushed a few commits cleaning up additional Python 2/3 compatibility blocks.

The python_warning.py module should probably be removed too.
'future' is still listed as a dependency even if its usage has been removed. Is that intended for potential extension/plugin?

In both cases, I limited the scope of this PR to non-breaking changes that would be amenable for a minor release. python_warning.py has been deprecated and is scheduled to be removed in the next major release of OMERO.py.

As for future, I can see an argument for dropping it even in a minor release as per https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api. Ideally, any downstream component that relies on future should have it declared. In practice, this will not necessarily the case and OMERO.web is a good example which does not declare future. How do you want to proceed here?

@will-moore
Copy link
Member

I was hoping to test with python 3.12 but I guess that's not available yet?

$ conda create -n omero-py312 -c ome python=3.12 zeroc-ice36-python
Collecting package metadata (current_repodata.json): done
Solving environment: failed with repodata from current_repodata.json, will retry with next repodata source.
Collecting package metadata (repodata.json): done
Solving environment: \ 
Found conflicts! Looking for incompatible packages.
This can take several minutes.  Press CTRL-C to abort.
failed                                                                                                                                                             

UnsatisfiableError: The following specifications were found to be incompatible with each other:

Output in format: Requested package -> Available versions

Package python conflicts for:
zeroc-ice36-python -> python[version='>=3.6,<3.7.0a0|>=3.7,<3.8.0a0|>=3.8,<3.9.0a0|>=3.9,<3.10.0a0']
python=3.12

@sbesson
Copy link
Member Author

sbesson commented Jan 31, 2024

@will-moore Glencoe Software is not using Conda so I cannot comment on the requirements to rebuild the zeroc-ice36-python recipe against a new set of Python targets.

In the spirit of the work announced recently, I have built a wheel package for testing these changes in a local Python 3.12 environment. The necessary additional patches are getting ported into the repository as we speak and we are working on getting new wheel packages for Python 3.12 released for Linux and macOS. For the latter, you might be able to give the artifacts of glencoesoftware/zeroc-ice-py-macos-universal2#2 a try.

@chris-allan
Copy link
Member

universal2 wheels for Ice on Python 3.12 have now been released:

@sbesson
Copy link
Member Author

sbesson commented Feb 2, 2024

The last few commits consume the latest release of the pre-built Ice wheel packages and add Python 3.12 to the testing matrix.

As indicated in https://docs.python.org/3/whatsnew/3.12.html, setuptools is no longer pre-installed in created virtual environments. 66ca6d9 updates the Tox requirements accordingly

The workflow logs have several SyntaxWarning related to the presence of backlash characters. Quite a few of these warnings come from the generated Python code in omero-blitz and more specifically the docstrings. I propose to review and update all the relevant strings in this repository and capture the rest as an issue on omero-blitz.

@chris-allan
Copy link
Member

The workflow logs have several SyntaxWarning related to the presence of backlash characters. Quite a few of these warnings come from the generated Python code in omero-blitz and more specifically the docstrings. I propose to review and update all the relevant strings in this repository and capture the rest as an issue on omero-blitz.

Similar to these, @sbesson?

zeroc-ice/ice#1560

@sbesson
Copy link
Member Author

sbesson commented Feb 2, 2024

Exactly. We have quite a few of them in the docstrings of the slice generated API classes, most of them introduced in ome/openmicroscopy#4349.

@chris-allan
Copy link
Member

chris-allan commented Feb 2, 2024

manylinux_2_28 (not usable on RHEL 7) wheels are also now available for Python 3.12:

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been able to test locally with Python 3.12 but the newly added tests are green.
Browsing the code changes looks good - can't say I've checked every line but from what I see it's 👍

@sbesson
Copy link
Member Author

sbesson commented Feb 21, 2024

@pwalczysko is there a quick summary of the functional testing that could be pasted here (and cross-referenced in ome/omero-web#531)?

@pwalczysko
Copy link
Member

@pwalczysko is there a quick summary of the functional testing that could be pasted here (and cross-referenced in ome/omero-web#531)?

Testing was performed through https://docs.google.com/spreadsheets/d/1zI3vlcTLp7okz_Mi2LHK40NxwCkGXUGHgDqgCd7jP_A/edit?usp=sharing and there were no major issues connected with this PR.

The minor issues were concerning the teting setup and older parts of the codebase.

@pwalczysko pwalczysko merged commit 8e53478 into ome:master Feb 22, 2024
8 checks passed
@sbesson sbesson deleted the future_removal branch February 22, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Remove future dependency
6 participants