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

Python 3.12 support / imp module deleted #1669

Merged
merged 6 commits into from
Jan 24, 2024

Conversation

EvanBldy
Copy link
Contributor

@EvanBldy EvanBldy commented Oct 9, 2023

Link the Issue(s) this Pull Request is related to.

#1668

Summarize your change.

The imp module in Python is deprecated since Python 3.4 and is removed in Python 3.12. So the package cannot be imported in Python 3.12 because the imp module doesn't exist anymore. See my issue.

By reading the docs it have to be changed by importlib. So I have made these changes and this works for importing adapters for example :)

Some documentation :
https://docs.python.org/3.12/whatsnew/3.12.html#imp
https://docs.python.org/3.11/library/imp.html

I have added Python 3.12 in the workflow for Python package.

I use some linter (black) in my vscode so my changes can be more than the real modifications. If you want I can revert these changes.

Reference associated tests.

Don't have launched the tests, they will run via GitHub Actions, no ?

I'm sorry it's my first PR to OpenTimeLineIO :)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 9, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@JeanChristopheMorinPerso
Copy link
Member

Hi @EvanBldy , it would be preferable if you remove any whitespace changes and formatting changes if possible.

@EvanBldy EvanBldy force-pushed the main branch 2 times, most recently from 82bda96 to e4e2150 Compare October 9, 2023 23:32
@EvanBldy
Copy link
Contributor Author

EvanBldy commented Oct 9, 2023

Hi @EvanBldy , it would be preferable if you remove any whitespace changes and formatting changes if possible.

It should be better :)

@EvanBldy EvanBldy force-pushed the main branch 3 times, most recently from 387bde2 to 46b29f0 Compare October 10, 2023 00:05
@EvanBldy
Copy link
Contributor Author

The tests are passing in Python 3.8 but not in 3.12, I don't understand the error neither what this code is intended to (https://github.com/AcademySoftwareFoundation/OpenTimelineIO/actions/runs/6462945630/job/17545363452?pr=1669)

@reinecke
Copy link
Collaborator

Hi @EvanBldy, thank you for working on this!

If you want to try running the tests locally, the usual process is to use these commands to setup and activate a development virtual environment:

python3 -m venv .venv
source .venv/bin/activate
pip install -e '.[dev]'  # This builds and installs OTIO and the development dependencies in your virtualenv

Depending on your setup, you may want to do python3.12 -m venv .venv to ensure the environment is built with the exact python version you want.

Then, each time you make a change, you can run:

python setup.py install  # This rebuilds OTIO with the changes and installs it into the env - goes quicker after the first time
make ci-prebuild  # This is what CI uses to lint and make sure all the extra data files needed are included
make test  # A shortcut for running the test suite
make ci-postbuild  # This is where the CI error was coming from

That should help you see what will happen in CI locally.

In terms of what autogen_serialized_datamodel.py does, that script autogenerates the markdown for this file and this file.

Digging into the error a bit more, I think the issue had to do with some internals of how a module is converted into a string. The important part of the trace is here:

  File "/opt/hostedtoolcache/Python/3.12.0/x64/lib/python3.12/site-packages/opentimelineio/console/autogen_serialized_datamodel.py", line 206, in <lambda>
    key=lambda mod: str(mod)
                    ^^^^^^^^
  File "<frozen importlib._bootstrap>", line 545, in _module_repr
  File "<frozen importlib._bootstrap>", line 830, in _module_repr_from_spec
AttributeError: 'VendorImporter' object has no attribute '_path'

This block of code is meant to traverse down into all the submodules. It's hitting some hiccup when it encounters pkg_resources.extern.VendorImporter. Upon further inspection, the block of code will actually dive into modules that OpenTimelineIO imports, not just OpenTimelineIO submodules.

To address this, I did a small rewrite of that block of code:

new_mods = sorted(
    (
        thing for _, thing in inspect.getmembers(mod)
        if (
            inspect.ismodule(thing)
            and thing not in modules
            and "opentimelineio" in thing.__name__
            and all(not thing.__name__.startswith(t) for t in SKIP_MODULES)
        )
    ),
    key=lambda mod: str(mod)
)

The main part is that I added an extra filter condition in there to only get modules that are part of opentimelineio. Testing locally, this seems to work for me. Feel free to give that a whirl and update the PR - I think that should fix it up!

@EvanBldy
Copy link
Contributor Author

Hi @reinecke,
Thanks for all the informations :)
And yeah the extra filter condition seems to do the trick.
I've updated my PR with this code and some other to resolve some "DeprecationWarning: Testing an element's truth value will raise an exception in future versions.".

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d98f657) 79.84% compared to head (9f40032) 79.84%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1669   +/-   ##
=======================================
  Coverage   79.84%   79.84%           
=======================================
  Files         197      197           
  Lines       21792    21796    +4     
  Branches     4358     4358           
=======================================
+ Hits        17399    17403    +4     
  Misses       2232     2232           
  Partials     2161     2161           
Flag Coverage Δ
py-unittests 79.84% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...timelineio/console/autogen_serialized_datamodel.py 78.51% <ø> (ø)
...timelineio/opentimelineio/plugins/python_plugin.py 89.55% <100.00%> (+0.66%) ⬆️
src/py-opentimelineio/opentimelineio/url_utils.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d98f657...9f40032. Read the comment docs.

Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

One test is failing on Windows with 3.12 (https://github.com/AcademySoftwareFoundation/OpenTimelineIO/actions/runs/6677197744/job/18146955106?pr=1669#step:9:24). Also I left a note about Pybind11.

.github/workflows/python-package.yml Show resolved Hide resolved
@EvanBldy
Copy link
Contributor Author

One test is failing on Windows with 3.12 (https://github.com/AcademySoftwareFoundation/OpenTimelineIO/actions/runs/6677197744/job/18146955106?pr=1669#step:9:24). Also I left a note about Pybind11.

Yeah, I'm trying to know what's the problem :) The test seems to be buggy only on Windows / Python 3.12, probably a new change in Python 3.12

@EvanBldy EvanBldy force-pushed the main branch 6 times, most recently from b456f6b to b3ca8d5 Compare October 30, 2023 00:05
@EvanBldy
Copy link
Contributor Author

EvanBldy commented Oct 30, 2023

It should be fixed :)
EDIT : There's now a problem with pypa/cibuildwheel, trying to upgrade it to support Python 3.12 Fixed

@EvanBldy
Copy link
Contributor Author

EvanBldy commented Nov 2, 2023

Hi @JeanChristopheMorinPerso @reinecke, are you okay with my PR ?
We use OTIO in our project and want to support Python 3.12, when do you plan to do a new release?

@EvanBldy
Copy link
Contributor Author

EvanBldy commented Jan 8, 2024

Hi ! Do you have news about this ?

@reinecke
Copy link
Collaborator

Hi @EvanBldy -
Sorry it took us a while to get back to you.
The change looks pretty good, but I saw the following files were modified with fixes unrelated to this PR:

Are those intentional? We prefer to keep PRs targeted at one specific thing. Would it be possible to revert those files and submit the changes to them as separate PRs?

Otherwise this is a great update and I'd love to get it in before the 0.16.0 release.

@reinecke reinecke self-assigned this Jan 10, 2024
@reinecke reinecke added this to the Public Beta 16 milestone Jan 10, 2024
@reinecke reinecke added maintenance General cleanup and good code hygiene python Pull requests that update Python code labels Jan 10, 2024
@EvanBldy
Copy link
Contributor Author

EvanBldy commented Jan 11, 2024

Hi @reinecke, thanks for your answer.

For the changes :

Cool if it can be in the 0.16.0 release.

@EvanBldy
Copy link
Contributor Author

I have removed the changes about DeprecationWarning and made a separate PR : #1688

Copy link
Collaborator

@reinecke reinecke left a comment

Choose a reason for hiding this comment

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

Thank you for all your work on this!

@reinecke reinecke merged commit 7302ff1 into AcademySoftwareFoundation:main Jan 24, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance General cleanup and good code hygiene python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants