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

BLD: Use modern jupyter_packaging and jupyterlab #111

Merged
merged 18 commits into from
Dec 23, 2024

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Nov 4, 2024

This is so bqplot-image-gl can build on Python 3.13

Also see similar patch at bqplot/bqplot#1653

xref spacetelescope/jdaviz#3210

cc @maartenbreddels @astrofrog

@pllim
Copy link
Contributor Author

pllim commented Nov 4, 2024

Okay so maybe you need to fix this up for jupyterlab 4? That is beyond my skillset here...

@dhomeier
Copy link
Contributor

dhomeier commented Nov 4, 2024

Looks like

"@jupyterlab/builder": "^3.0.0-rc.4",

still pins the npm install to 3.x. No idea if it can safely bumped to ^4.0.0

as suggested by dhomeier
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.44%. Comparing base (7b90b9d) to head (f9287c7).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
- Coverage   50.00%   44.44%   -5.56%     
==========================================
  Files           7        8       +1     
  Lines         140      189      +49     
==========================================
+ Hits           70       84      +14     
- Misses         70      105      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pllim
Copy link
Contributor Author

pllim commented Nov 4, 2024

Oh, thanks, @dhomeier ! CI is green now. :D

@maartenbreddels
Copy link
Collaborator

This looks overal good. Given that we run the visual tests, it seems we can build against jupyter lab 4. Although, I would rather not see us drop 36-38 without a good reason, can we put those back in?

@pllim
Copy link
Contributor Author

pllim commented Nov 12, 2024

Python 3.6-3.8 have reached EOL. You cannot get them on Actions anymore.

@pllim
Copy link
Contributor Author

pllim commented Nov 12, 2024

And even if you can, you will be stuck with a very old stack with no security patches.

@maartenbreddels
Copy link
Collaborator

We still run it every day without a problem: https://github.com/widgetti/solara/actions/runs/11791722711/job/32852280380

I’m a strong advocate against proactively dropping support for older Python versions. In many cases, EOL alone isn’t a sufficient reason to discontinue support, especially in regular user environments without public HTTP servers, where security risks are minimal. Of course, if maintaining compatibility becomes a burden, that’s a valid reason to consider dropping it. But so far, almost all CI I manage runs smoothly on Python 3.6 and above. Admittedly, 3.6 is old, but you’d be surprised how frequently these environments are still in use!”

In any case, apart from this discussion, this PR is about adding 3.13 support, not removing older versions. If you feel very strongly about removing support of older versions, I think it should go into a different PR.

because Maarten wanted them back [ci skip]
@pllim
Copy link
Contributor Author

pllim commented Nov 12, 2024

Okay I added them back but fixing them is not in my plans.

Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

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

This may still be running up to a decision between supporting Python 3.13 and 3.6-3.7. Could consider conditional requires, but I'd like to check if the version requirements really need to be that strict – moving to 0.12/4.x did not require any actual code changes, right? So I suggest to try this as a (final?) attempt to keep the older versions.

I have also tried to probe into the failures with py37-macos12, and could not reproduce the installation problems on macOS 10.14, but was unable to build bqplot-image-gl either since pip did not find the npm installation. Perhaps move the py37 tests to a linux host if nothing else helps?

pyproject.toml Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor Author

pllim commented Nov 13, 2024

If you insist on supporting very old Python, can you please take over this PR? This is getting very complicated and out of scope of my work to just get this package to build for Python 3.13. Thanks.

Co-authored-by: Derek Homeier <[email protected]>
@pllim
Copy link
Contributor Author

pllim commented Dec 19, 2024

py36 still fails with suggestion from @dhomeier but I think you should really drop it. It is very, very old by now.

@iisakkirotko
Copy link
Collaborator

iisakkirotko commented Dec 20, 2024

@pllim jupyterlab dropped support for Python 3.6 before version 3.6, in 3.3.0. Maybe changing the requirement to >=3.0 or >=3.2 will fix the issue.

js/package.json Outdated Show resolved Hide resolved
@dhomeier
Copy link
Contributor

If the switch to hatch setup in #113 will remove 3.6 support anyway, +1 to dropping it here already

@iisakkirotko
Copy link
Collaborator

Hey @pllim and @dhomeier, me and @maartenbreddels discussed this a bit today, and we think that getting to use the new build process in #113 is a good chance to drop Python 3.6 support. So feel free to drop 3.6 here, and we can get this, #113, and #112 in!

jobs:
build:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

One last thing I've been wondering about, here and below: do they really need to be pinned to a specific commit?
Seems not very maintenance-friendly for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a security thing. Dependabot still works with hash pin.

@pllim
Copy link
Contributor Author

pllim commented Dec 20, 2024

ImportError: cannot import name '_build_artifact_test_folder' from 'pytest_playwright.pytest_playwright'

I don't know what this is about... not related to my PR?

@pllim
Copy link
Contributor Author

pllim commented Dec 20, 2024

packages/pip/_vendor/typing_extensions.py", line 1039
    def TypedDict(typename, fields=_marker, /, *, total=True, closed=False, **kwargs):
                                            ^

I cannot get py37 to run either. Do you also have plans to drop it? Even Python 3.8 is EOL now but it is not a hill I want to die on.

@dhomeier
Copy link
Contributor

py37 + macos-12/macos-13 is already broken in other runs, not really within the scope of this PR to fix it. Perhaps move the job to ubuntu as suggested earlier?

@dhomeier
Copy link
Contributor

Should be available on

   "platform_version": "22.04",
   "download_url": "https://github.com/actions/python-versions/releases/download/3.7.17-5356448435/python-3.7.17-linux-22.04-x64.tar.gz"

as well, but this works at least!

@pllim
Copy link
Contributor Author

pllim commented Dec 20, 2024

OK I bumped the Ubuntu version. Thanks!

At this point, I think I addressed all the concerns and comments?

Please use the "squash and merge" button.

@iisakkirotko
Copy link
Collaborator

The remaining issues:

ImportError: cannot import name '_build_artifact_test_folder' from 'pytest_playwright.pytest_playwright'

and

packages/pip/_vendor/typing_extensions.py", line 1039
    def TypedDict(typename, fields=_marker, /, *, total=True, closed=False, **kwargs):
                                            ^

are both addressed in #113. I should have created separate PRs for them, now that PR is a bit of a mess of slightly unrelated changes.

@maartenbreddels maartenbreddels merged commit 5c8af3b into glue-viz:main Dec 23, 2024
14 of 16 checks passed
@maartenbreddels
Copy link
Collaborator

Thanks all!

@pllim pllim deleted the patch-1 branch December 23, 2024 14:01
@pllim
Copy link
Contributor Author

pllim commented Dec 23, 2024

Thanks for all your help and reviews!

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.

4 participants