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

Drop 3.6 and 3.7 and support 3.11 and 3.12 #2735

Merged
merged 22 commits into from Feb 8, 2024

Conversation

graingert-coef
Copy link
Contributor

@graingert-coef graingert-coef commented Jan 31, 2024

Fixes #2736

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • update ci to support 3.12
    • update ci to support 3.8
    • update package metdata to support 3.8-3.12
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@graingert-coef graingert-coef force-pushed the drop-3-6-support-3-12 branch 5 times, most recently from 98ef6b4 to 81ab7b5 Compare January 31, 2024 17:45
@alexcjohnson alexcjohnson mentioned this pull request Feb 2, 2024
8 tasks
@graingert
Copy link
Contributor

graingert commented Feb 5, 2024

@alexcjohnson when running the integration tests locally I get: ModuleNotFoundError: No module named 'dash_generator_test_component_nested' do you know how I can install those?

Ah it's npm run setup-tests.py

@graingert-coef graingert-coef force-pushed the drop-3-6-support-3-12 branch 2 times, most recently from 3098285 to ddf7a1b Compare February 6, 2024 10:22
@graingert-coef graingert-coef marked this pull request as ready for review February 6, 2024 10:46
@@ -49,7 +49,7 @@ jobs:
name: 🏁 Build Component Packages & Update Dependencies/Artifacts
command: |
python -m venv venv && . venv/bin/activate
pip install --upgrade pip wheel
pip install --upgrade pip wheel setuptools
Copy link
Contributor

Choose a reason for hiding this comment

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

ok the reason this works took me a few days to workout, but basically celery prepends the current working directory to sys.path if it's not already present. This means the local source tree takes priority over installed modules so "import tests" works but "import dash" fails with a missing attribute on dash.dcc

When setuptools and wheel is installed pip will use "setup.py develop" which adds the current sys.path to site-packages/easy-install.pth which appends the current directory so the local source tree is available but installed modules take priority, this means "import tests" works and so does "import dash"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding setuptools here is just a workaround that causes pip to fallback to legacy editable project mode, I have a fix for modern editables here #2749

@graingert-coef
Copy link
Contributor Author

@alexcjohnson finally all the tests are passing! What do you think?

@graingert-coef
Copy link
Contributor Author

@alexcjohnson should I rebase this off of dev?

@alexcjohnson
Copy link
Contributor

@alexcjohnson should I rebase this off of dev?

Yep! Then I'll give it a final review 🎉

@graingert-coef graingert-coef force-pushed the drop-3-6-support-3-12 branch 3 times, most recently from 6ccf221 to d9cdaaf Compare February 6, 2024 17:49
use py -3.12 -m to run pip

call render.exe via Python312/Scripts dir
preconditions has been unmaintained for 9 years and calls the
removed function inspect.getargspec
handle getfullargspec .keywords renamed to .varkw
pylint 3 will fail if all rules are disabled!
It looks like all the rules got disabled in plotly@65a1d5c#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R11 so I added back as many rules that still pass.
I'll restore the remaining rules in a followup PR
use passed in browser to distinguish browser type
update smoke test to use .startswith()
previously exceptions were parsed line by line, however
3.11 introduced https://docs.python.org/3/whatsnew/3.11.html#whatsnew311-pep657
which means that exception line numbers and traceback skip offsets
no longer line up.

Rather than inspect the source code this change introduces an
extra frame in `def _invoke_callback` that can be detected
by walking the traceback and finding its code object.
@graingert-coef
Copy link
Contributor Author

@alexcjohnson ok I've rebased

.pylintrc312 Outdated Show resolved Hide resolved
@@ -15,7 +15,7 @@
"private::format.black": "black dash_core_components_base/ tests/ setup.py",
"private::format.eslint": "eslint src --fix",
"private::format.prettier": "prettier --config .prettierrc --write src/**/*.js",
"private::lint.black": "node -e \"if ((process.env.PYVERSION || 'python310') !== 'python36'){process.exit(1)} \" || black --check dash_core_components_base/ tests/ setup.py",
"private::lint.black": "node -e \"if ((process.env.PYVERSION || 'python312') !== 'python38'){process.exit(1)} \" || black --check dash_core_components_base/ tests/ setup.py",
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we can leave this as you have it for now, but way better would be to pick a version of black that supports all the same Python versions we do and always run it. The only reason we didn't do that from the start is that when we first introduced black we still supported Py2.7 and black was always Py3-only.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do this in a follow up PR

requires-ci.txt Outdated Show resolved Hide resolved
Co-authored-by: Alex Johnson <alex@plot.ly>
Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Beautiful!

@alexcjohnson alexcjohnson merged commit 18960af into plotly:dev Feb 8, 2024
3 checks passed
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.

[Feature Request] Python 3.12 support
3 participants