Skip to content

build+docs: use Python 3.11 and pin a recent version of pytest #4605

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 13, 2024
Merged

build+docs: use Python 3.11 and pin a recent version of pytest #4605

merged 1 commit into from
May 13, 2024

Conversation

gvwilson
Copy link
Contributor

@gvwilson gvwilson commented May 7, 2024 •
edited
Loading

  • conda create -n plotly-dev python currently gives Python 3.12.3.
  • pytest -v packages/python/plotly/plotly/tests/ then fails with ModuleNotFoundError: No module named 'imp'.
    • pip install imp fails: the imp module was deprecated in Python 3.11 and has been removed in Python 3.12.
  • So update contributing.md to specify conda create -n plotly-dev python=3.11
  • But now pytest fails
    • File "<frozen importlib._bootstrap>", line 1072, in _find_spec
    • AttributeError: 'AssertionRewritingHook' object has no attribute 'find_spec'
  • Problem was pytest==3.5.1 in packages/python/plotly/optional-requirements.txt
  • Update to pytest==8.1.1 and pytest runs
    • Some tests are failing (orca problems, statsmodels not installed, etc.)
    • Will fix these in separate PRs
  • Note: updating pytest also solves the problem with the imp module not being available in Python 3.12.

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.

closes #4591

@gvwilson gvwilson self-assigned this May 7, 2024
emilykl
emilykl approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

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

All good as far as I'm concerned. Thanks @gvwilson .

Have you been able to determine yet which dependency(ies) relies on imp?

@gvwilson
Copy link
Contributor Author

gvwilson commented May 7, 2024

@emilykl not yet - I'd like to fix the broken tests on Python 3.11 first, then switch back to Python 3.12 and tackle the imp issue. Good news is that pyzmq 26.0.3 seems to install with Python 3.12.

archmoj
archmoj reviewed May 7, 2024
View reviewed changes
CHANGELOG.md

### Updated
- Specify Python 3.11 in virtual environment for development and pin `pytest` at version 8.1.1 to match

Copy link
Contributor

Choose a reason for hiding this comment

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

@LiamConnors which version we used for the last plotly.py release?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also wondering perhaps we wanna mention the version in release docs below? https://github.com/plotly/plotly.py/blob/master/release.md

Copy link
Member

Choose a reason for hiding this comment

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

The release build uses 3.9 and the docs build also currently uses 3.9, though I have a PR to upgrade the docs build to 3.10

Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://docs.python.org/3/whatsnew/3.11.html
3.11 seems to run faster when compared to 3.10.

Do you think we could/should upgrade those to 3.10 or 3.11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would you like me to check that the newly-pinned version of pytest (8.1.1) works with Python 3.9 and Python 3.10 as well as Python 3.11? contributing.md explicitly mentions Python 3.6 and Python 3.9 in conjunction with tox, which I haven't been using so far, but I could set that up and test it later today or tomorrow. RSVP.

Copy link
Member

Choose a reason for hiding this comment

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

That would be great.
Yeah, I guess the tox stuff is potentially out of date now. I haven't tried those steps https://github.com/plotly/plotly.py/blob/master/contributing.md#running-tests-with-tox but I'll try with a few different Python versions.

@archmoj archmoj requested a review from LiamConnors May 7, 2024 15:38
LiamConnors
LiamConnors reviewed May 7, 2024
View reviewed changes
contributing.md Outdated
conda activate plotly-dev
```

Please note that our dependencies require Python 3.11 as of May 2024.
Copy link
Member

Choose a reason for hiding this comment

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

I think it should still work with Python3.8 - 3.10.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm testing with Python 3.8-3.10 now…

@gvwilson
Copy link
Contributor Author

gvwilson commented May 7, 2024 •
edited
Loading

Results on MacOS Sonoma 14.4.1 - I will update my PR tomorrow to (a) pin the pytest version and (b) modify contributing.md to say this has been tested on Python 8-11. Still have failing tests in several areas; I'll dig into those as well in separate PRs.

Python version existing pytest==3.5.1 updated pytest==8.1.1
3.8.19 fails: ImportError: cannot import name 'Config' from 'pytest' succeeds
3.9.19 fails: ImportError: cannot import name 'Config' from 'pytest' succeeds
3.10.14 fails: TypeError: required field "lineno" missing from alias succeeds
3.11.9 fails: TypeError: required field "lineno" missing from alias succeeds

@gvwilson
Copy link
Contributor Author

gvwilson commented May 8, 2024

PR updated: @LiamConnors can you please take a look at contributing.md and CHANGELOG.md and let me know?

LiamConnors
LiamConnors reviewed May 8, 2024
View reviewed changes
CHANGELOG.md Outdated
## [5.23.0] - TBD

### Updated
- Specify Python version 8-11 for development virtual environments and pin `pytest` at version 8.1.1 to match.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Specify Python version 8-11 for development virtual environments and pin `pytest` at version 8.1.1 to match.
- Specify Python version 3.8-3.11 for development virtual environments and pin `pytest` at version 8.1.1 to match.

LiamConnors
LiamConnors approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@LiamConnors LiamConnors left a comment

Choose a reason for hiding this comment

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

Looks good

@gvwilson
build+docs: use Python 3.8 - 3.11 and pin pytest 8.1.1
9fdd6b6 
-   `conda create -n plotly-dev python` currently gives Python 3.12.3.
-   `pytest -v packages/python/plotly/plotly/tests/` then fails with `ModuleNotFoundError: No module named 'imp'`.
    -   `pip install imp` fails: the `imp` module was deprecated in Python 3.11 and has been removed in Python 3.12.
-   So update `contributing.md` to specify `conda create -n plotly-dev python=3.11`
-   But now `pytest` fails
    -   `File "<frozen importlib._bootstrap>", line 1072, in _find_spec`
    -   `AttributeError: 'AssertionRewritingHook' object has no attribute 'find_spec'`
-   Problem was `pytest==3.5.1` in `packages/python/plotly/optional-requirements.txt`
-   Update to `pytest==8.1.1` and `pytest` runs
    -   Some tests are failing (orca problems, `statsmodels` not installed, etc.)
    -   Will fix these in separate PRs
-   Update `contributing.md` to state that we've tested changes against Python 3.8-3.11
    -   Specifically 3.8.19, 3.9.19, 3.10.14, and 3.11.9 on MacOS Sonoma 14.4.1
-   Update `CHANGELOG.md`
@gvwilson
Copy link
Contributor Author

gvwilson commented May 8, 2024

@LiamConnors pushed a fix for the CHANGELOG (sorry about 8-11 instead of 3.8-3.11) - rebased to put it in the same commit as the other changes to hide my silly typo.

@gvwilson gvwilson merged commit c604379 into plotly:master May 13, 2024
4 checks passed
@gvwilson gvwilson deleted the use-python-3.11-and-pytest-8.1.1 branch May 13, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archmoj archmoj archmoj left review comments

@emilykl emilykl emilykl approved these changes

@LiamConnors LiamConnors LiamConnors approved these changes

Assignees

@gvwilson gvwilson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

update development instructions for MacOS
4 participants
@gvwilson @emilykl @LiamConnors @archmoj