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 deprecation fixes (including Preparing for Python 3.9) #820

Merged

Conversation

reinecke
Copy link
Collaborator

@reinecke reinecke commented Oct 13, 2020

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

Addressed deprecation warning:

opentimelineio/core/_core_utils.py:220: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.9 it will stop working
  if not isinstance(item, collections.Iterable):

We do this by importing abc from collections where available (Python 3.3+), otherwise we alias collections as abc.

This fixes OTIO in Python 3.9.

Also addressed:

  • Add __next__ (favored instead of next in Python 3.0+) - PEP 3114
  • Where available (Python 3.0+) use inspect.getfullargspec rather than inspect.getargspec

Reference associated tests.

Existing tests covered the use cases already, under Python 3.9 they were failing. I successfully ran the test suite in Python 3.9 locally and verified the deprecation warnings are no longer dropping to the shell in other versions.

Python 3.9.0 support is not explicitly added due to a known intermittent crash that can be caused by the combination of Python 3.9.0 and pybind11. See #828 for more context.

@codecov-io
Copy link

codecov-io commented Oct 13, 2020

Codecov Report

Merging #820 into master will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #820      +/-   ##
==========================================
+ Coverage   84.02%   84.18%   +0.16%     
==========================================
  Files          74       74              
  Lines        3124     3061      -63     
==========================================
- Hits         2625     2577      -48     
+ Misses        499      484      -15     
Flag Coverage Δ
#py27 ?
#py37 ?
#py38 ?
#unittests 84.18% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...pentimelineio/opentimelineio-bindings/otio_utils.h 75.80% <100.00%> (+1.20%) ⬆️
...ntimelineio/opentimelineio-bindings/otio_utils.cpp 86.90% <0.00%> (-0.46%) ⬇️
src/opentimelineio/typeRegistry.cpp 84.84% <0.00%> (-0.45%) ⬇️
...entimelineio-bindings/otio_serializableObjects.cpp 95.11% <0.00%> (-0.31%) ⬇️
src/opentimelineio/item.cpp 90.47% <0.00%> (-0.15%) ⬇️
...lineio/opentime-bindings/opentime_rationalTime.cpp 97.36% <0.00%> (-0.07%) ⬇️
src/opentimelineio/serialization.cpp 83.60% <0.00%> (-0.06%) ⬇️
src/opentimelineio/timeline.h 100.00% <0.00%> (ø)
src/opentimelineio/typeRegistry.h 100.00% <0.00%> (ø)
...imelineio/opentime-bindings/opentime_timeRange.cpp 100.00% <0.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

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

Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

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

Out of curiosity is the name "abc" a reasonable acronym for something, or is it some legacy hackiness? If hackiness, I wonder if there's some nicer name to import as. If I am reading "collections.Iterable" it's readable, but reading "abc.Iterable" is kind of out of left field for someone like me who isn't deep into Python arcana :)

@apetrynet
Copy link
Contributor

abc -> abstract base classes

@meshula
Copy link
Collaborator

meshula commented Oct 14, 2020

Ah, that does sound overly generic to be honest since that's a basic computer science term that doesn't tell us anything about what's being abstracted beyond a "class", which isn't what actually going on here. My personal preference would be that we use a descriptive name.

@reinecke
Copy link
Collaborator Author

I agree about the abc thing feeling a bit like arcana. Ultimately, I selected this approach because:

  1. It avoids inventing a new name for the module
  2. The main impact is in a core module that's implementing stuff mostly focussed on leveraging these collections.abc classes so hopefully the readers are more inclined to be familiar with them.

Looking at the PEP that introduced this module (https://www.python.org/dev/peps/pep-3119/), it turns out there is just a root level abc, so my approach could cause confusion i.e. import abc vs. from collections import abc.

Looking at how six handles this, it uses the rename collections_abc. Given that all this code is doing is trying to achieve what six does without adding a dependency, I think I'll update mine to match it's behavior.

Side question for the community: How much of a problem would adding a dependency on six be? It's considered the standard for many of the Python 2/3 interoperability problems and would also flag the places in code that can be scrubbed when we drop Python 2 support.

Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

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

Thanks for the change!

@meshula
Copy link
Collaborator

meshula commented Oct 14, 2020

The six library is a lightweight and robust dependency, in my experience ~ I'm not a Python guru, but six has never caused me a maintenance issue, so with that caveat, it gets a thumbs up from me.

@reinecke
Copy link
Collaborator Author

I'm going to drop the six library thing into an issue to allow that discussion to have it's own place.

Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

One question in here, and I haven't followed the discussion fully, but this fixes the code but doesn't add the CI support - is that a mistake? If we're going to fix the code, should we also just add CI support? It seems like we're committing to supporting Python3.9 at that point? That would also imply updating the badge.

src/py-opentimelineio/opentimelineio/adapters/adapter.py Outdated Show resolved Hide resolved
…uilds - added 3.9 and corrected it still having 3.6 and missing 3.8.
…ch the python 3 naming rather than python 2.
…e context about the Python 3.9.0/pybind11 bug.
@reinecke reinecke changed the title Python 3 deprecation fixes (including Python 3.9 support) Python 3 deprecation fixes (including Preparing for Python 3.9) Oct 30, 2020
Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

This is great, thanks.

We can integrate the 6 stuff in a future PR.

Thanks Eric!

@ssteinbach ssteinbach merged commit 44f8ce8 into AcademySoftwareFoundation:master Oct 30, 2020
@ssteinbach ssteinbach added this to the Public Beta 14 milestone Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants