build: refactor build strategy #259

12rambau · 2023-04-21T10:39:10Z

W.I.P but should fix #256

move everything I can to pyproject.toml as it has won the configuration war
remove warning, raise etc... from setup..py as they are ignored since setuptools 50
use an python package for NPM binding removing many extra line from the setup.py page

I clearley identify the from generate_source import generate as the culprit but I don't yet manage to find a way to load it BEFORE the end of the installation process. It seems to be unavailable in the isolated environment even if it's part of the egg_info folder.

TODO

solve the installation issue
check that all data files are still there

12rambau · 2023-04-21T11:44:01Z

I think what we try to achieve here (igenerate the source upon installation) is not possible when the lib is build in isolation as the generate_source module is not yet available in the venv.

Maybe an alternative solution could be:

for release:
- generate the source
- generate the wheel (with the sources inside)
- publish
for local build:
- create a nox session (build: use nox for tests and docs #240) to generate the sources for local dev.
- explain the process in the contribute section of the doc

@mariobuikhuizen, @maartenbreddels what do you think ?

maartenbreddels · 2023-04-21T11:58:54Z

not ideal, i've always though a pip install should work. Why doesn't the isolated build pick up the generate source code, especially since it is in the manifest file, it should be available (maybe in a different directory.. ?)

maartenbreddels · 2023-04-25T14:34:38Z

Did you rebase to get 778437a in?

12rambau · 2023-04-25T15:14:42Z

I don't remember rebasing but it's already in. I'm discovering the deps of setuptools with pyca developers, the pip install now works but the python -m build doesn't.

pypa/setuptools#3909 (reply in thread)

12rambau · 2023-04-26T11:55:47Z

@maartenbreddels, I'm moving forward with the installation process. Still few things to optimize but at least it works (pip install, pip install -e and python build).

Things are still buggy in the tests though. I have several questions:

why is there a build job ? the produced artifacts are reused in all the others but they exactly correspond to the one produced by a python build, that could be relaunched (not that long) for every job and thus make them in parralel.
why not installing the lib the same way people are doing it with pip install .. refering to the build action artifact is the main reason last version was released with a non complete wheel. I think we should only rely on pip install for tests.
why not integrating the solara based test in a pytest suit ? so that it can be launched by developer before pushing ? I think it can be managed after build: use nox for tests and docs #240

If you agree with my comments I'll move forward with my PR and revamp the test actions, the release one will deserve a dedicated PR.

maartenbreddels · 2023-04-26T12:09:45Z

I'm moving forward with the installation process. Still few things to optimize but at least it works (pip install, pip install -e and python build).

Great news!

why is there a build job ?

I want the build step to be isolated, as to mimic as much as possible a release->user install process. We don't want that because of some artifact during build process, our tests succeed, but when installing the produced wheel it will not (this in fact caused a faulty release).

the produced artifacts are reused in all the others but they exactly correspond to the one produced by a python build

Yes, that's exactly what we want to test: Do the tests pass when the wheel that we build is released as it is. We don't "care" about the tests passing because we somehow managed to get it working.

why not installing the lib the same way people are doing it with pip install .

No user installs it this way right? It's all comping from pypi, which means people install the wheel.

why not integrating the solara based test in a pytest suit

Not sure what you mean by this? It's in the test directory right, if you run py.test tests it will run every test locally. The reason we split it, is because the UI tests are pretty expensive (long to run), and we don't need to test it for every Python version. Does that make sense?

maartenbreddels · 2023-04-26T12:11:00Z

PS: I'd rather have you rebase on master, then merge master back in, this is often confusing (at least for me) and leads to weird incomprehensible conflicts (again, at least for me:P)

12rambau · 2023-04-26T13:19:13Z

No user installs it this way right? It's all comping from pypi, which means people install the wheel.

if you run pip install . on a project that build a universal wheel with a pyproject.toml then the wheel is produced (at list its content in sdist) and used for the installation so that actually mimick more the installation from pypi than the current one. On the plus side you check if you included all the necessary requirements.

PS: I'd rather have you rebase on master, then merge master back in, this is often confusing (at least for me) and leads to weird incomprehensible conflicts (again, at least for me:P)

I know I'll try to make the test pass first and then rebase (which will be a pain for sure)

maartenbreddels · 2023-04-26T13:37:15Z

I will believe that pip install . should behave the same, but I think having a different build and test environment makes sure that you reproduce exactly how users run it.
Ideally, we also check the pip install . and pip install -e . path. But I think that will not affect end-users, so it's less important. Most important I think is to never break a user-install.

12rambau · 2023-04-26T13:56:28Z

I reintroduced a build step (I think it's a duplicate but beter safe than sorry), dropped 3.6 and all the test are passing. The solara based ui assesment is not working and as I have 0 knowledge, could you have a look ?

side note: I'm not 100% sure we need to check ui aspect of any widget as it will be displayed in the documentation anyway using jupyterlite.

maartenbreddels · 2023-04-26T14:13:07Z

I'm not 100% sure we need to check ui aspect of any widget as it will be displayed in the documentation anyway

This makes sure we can never have a broken release. If the UI tests pass, we know it's working on classic notebook, lab, voila and solara.

Why did you drop 3.6? We still need to support it. Are we using a non-supported feature in this PR?

maartenbreddels

We cannot see what is going wrong now, take a look at my comments so we can debug this.

.github/workflows/test.yml

12rambau · 2023-04-26T15:20:57Z

Let me advocated on some of your suggestions:

why don't I want to rely on the build wheel ?

Because it's strictly equivalent to running pip install . when using setuptools (confirmed in the thread where I talk with them) so building the wheel manually to save it and share it on each runner is making the actions more complex -> more dificult to maintain. The current matrix is only running on ubuntu but we could extend it to OSX and windows, it's much more dificult to share files between platform which would force to special case for windows. Finally for the tests, it's super easy to add the test requirements in extras (which is the case in this PR) which is not possible using the wheel. this what is done on all the repositories I'm working on and many of them include nodeJS deps, we never encountered any issue (https://github.com/pydata/pydata-sphinx-theme). I had the feeling that the number of concurrent jobs is fix so if they run all at once I'm still a bit faster.

Why do I think the solara test is overdoing it ?

I would like to use mystnb in the documentation which will build ALL the widgets. using pytest-regression, it will be super easy to check the produced html widgets. It will be equivalent to the test performed on solara without using playwrite which can be tricky (we are using it in pydata-sphinx-theme for a11y and that's is bugging on runners 1 time out of 2) and ensurin the test of all of them.

why do I want to drop 3.6 ?

According to python documentation, 3.6 have reached end-of-life for a long time now and has been dropped by multiple other libs.

https://devguide.python.org/versions/

12rambau · 2023-04-27T07:22:13Z

everything seems ok now !

maartenbreddels · 2023-04-27T07:50:01Z

😍

12rambau · 2023-06-13T12:20:18Z

@maartenbreddels did you had the time to have a look at the latest iteration ?

maartenbreddels · 2023-06-14T19:16:06Z

I've taken a quick look, it looks nice!
So cmdclass={"egg_info": js_prerelease(egg_info)} is the magic trick?

12rambau · 2023-06-14T20:23:54Z

it seems it covers all cases as any build type requires to create a egg_info folder first.

12rambau · 2023-07-17T21:04:45Z

I was crowling in my notification dashboard, and I saw this was still requesting changes. @maartenbreddels is it a legacy request or should I change anything ?

mariobuikhuizen

Thank you very much for fixing the build and sorry for the long delay.

Please try do fixes, adding linting, refactoring, etc in separate PR's next time. This makes them quicker/easier to review, discuss and merge.

A few small issues:

.github/workflows/test.yml

mariobuikhuizen · 2023-08-21T10:53:45Z

.github/workflows/test.yml

    strategy:
      fail-fast: false
      matrix:
-        python-version: [3.6, 3.7, 3.8, 3.9, "3.10", "3.11"]
+        python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"]


Why remove python 3.6? Our experience is that some users are still tied to 3.6 because of other dependencies or slow upgrade cycles in companies.

As 3.7 as already reached end of life, keeping the previous one that is already dead seems too much for me. if you would like to keep it please run the test again as I may have introduce some pathlib mechanism that were not available in 3.6

Sadly the reality is that 3.6 it's not dead, especially in big companies.

solved in f3980bb

it seems pytest-playwright is not compatible with 3.6 either, making the build fail. what should I do ?

I think we only need solara[pytest] and thus ${wheel}[test] in the ui-test job. Removing the [test] from $(find dist -name *.whl)[test] will fix it.

First I would like to solve the 3.6 build. all the others from 3.7 to 3.11 worked but 3.6 failed because playwright is not working. I cannot drop [test] there as it's where the test requirements are but if they are based on playwright it seems my life will become more complicated

I think you removed [test] from the ui-test instead of the test job.

you were talking about the ui-test so I checked. I cannot remove [test] from the normal test as solara itself is set in it.

Just to advocate again in favor of dropping python 3.6, many of the top used widget libs that I know (+other packages that are not widgets) have already dropped it:

ipywidgets: https://pypi.org/project/ipywidgets/ >=3.7

ipyleaflet: https://pypi.org/project/ipyleaflet/ >= 3.7

matplotlib: https://pypi.org/project/matplotlib/ >= 3.8

pandas: https://pypi.org/project/pandas/ >= 3.8

To make the test work on 3.6 we should redesign them without use of playwright which is use directly as a fixture.

At the end of the day it will continue to work with 3.6 we will just stop making bug fixes.

It's fixed with removing [test] in the test job. We can postpone dropping 3.6 support.

requirements.txt

toto.ipynb

.gitignore

Co-authored-by: Mario Buikhuizen <[email protected]>

This reverts commit 0140143.

Probably because of the recent release of Jupyter Lab 4

mariobuikhuizen · 2023-08-21T19:20:05Z

Should we squash and merge this?

12rambau · 2023-08-21T19:46:39Z

As you see fit, I personnaly don't mind. It will ease readability in main though

12rambau added 6 commits April 21, 2023 11:08

build: move information to pyproject.toml

e8600dd

refactor: move more things to pyproject.toml

5a0b012

refactor: move bdist_wheel config to pyproject.toml

9d551db

refactor: file cleaning

d4af14a

build: update the gitignore with all the generated files

67abd19

refactor: use pathlib in generate_source

703dde7

12rambau added 2 commits April 25, 2023 07:20

fix: set the path in sys explicitely

79b60df

build: manage files that should be included in the distrib

c96795e

12rambau added 3 commits April 26, 2023 10:49

refactor: avoid using single used class

24d3623

build: add js folder to manifest.in

69549be

build: fix action build

933e7c3

12rambau and others added 2 commits April 26, 2023 14:04

Merge branch 'master' into install

5e5ca6a

build(deps): set playwrite and solara in pyproject.toml

4ce25a8

test: update actions

825b055

build: run CI in parralel

7aa421e

test(CI): reintroduce a wheel build test

dc8aa2f

maartenbreddels requested changes Apr 26, 2023

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

.github/workflows/test.yml Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

build: restore export artifacts

2ea00a7

12rambau added 2 commits April 27, 2023 08:16

refactor: typo

ef98fda

buimd: only install npm packages upon egg-info creation

11ac604

12rambau marked this pull request as ready for review April 27, 2023 07:02

12rambau marked this pull request as draft April 27, 2023 07:02

12rambau marked this pull request as ready for review April 27, 2023 07:21

12rambau mentioned this pull request Jun 5, 2023

I don't manage to install jupyter in RDT build readthedocs/readthedocs.org#10223

Closed

refactor: remove the test notebook

ad5f5eb

12rambau mentioned this pull request Aug 6, 2023

update packaging jupyter-widgets/ipyleaflet#1131

Draft

mariobuikhuizen mentioned this pull request Aug 21, 2023

build: use nox for tests and docs #240

Merged

mariobuikhuizen requested changes Aug 21, 2023

View reviewed changes

12rambau and others added 9 commits August 21, 2023 14:56

Update .gitignore

23c1b9b

Co-authored-by: Mario Buikhuizen <[email protected]>

fix: pin ubuntu version in CI

e84b4b2

fix: restore binder requirements

f996604

build: support Python from 3.6 to 3.11

f3980bb

build(ci): use the default install for ui-test

0140143

Revert "build(ci): use the default install for ui-test"

1ec1816

This reverts commit 0140143.

build(ci): fix test job for python 3.6 by removing playwright dep

95cb6b4

build(ci): fix failing Jupyter Lab ui test

cfdeaaf

Probably because of the recent release of Jupyter Lab 4

build(ci): fix "no such kernel" error on readthedocs

d6c7200

mariobuikhuizen approved these changes Aug 21, 2023

View reviewed changes

mariobuikhuizen merged commit 10c2dfe into widgetti:master Aug 22, 2023

12rambau deleted the install branch August 22, 2023 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: refactor build strategy #259

build: refactor build strategy #259

12rambau commented Apr 21, 2023 •

edited

Loading

12rambau commented Apr 21, 2023 •

edited

Loading

maartenbreddels commented Apr 21, 2023

maartenbreddels commented Apr 25, 2023

12rambau commented Apr 25, 2023 •

edited

Loading

12rambau commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

12rambau commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

12rambau commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

maartenbreddels left a comment

12rambau commented Apr 26, 2023 •

edited

Loading

12rambau commented Apr 27, 2023

maartenbreddels commented Apr 27, 2023

12rambau commented Jun 13, 2023

maartenbreddels commented Jun 14, 2023

12rambau commented Jun 14, 2023

12rambau commented Jul 17, 2023

mariobuikhuizen left a comment

mariobuikhuizen Aug 21, 2023

12rambau Aug 21, 2023

mariobuikhuizen Aug 21, 2023

12rambau Aug 21, 2023

12rambau Aug 21, 2023

mariobuikhuizen Aug 21, 2023

12rambau Aug 21, 2023

mariobuikhuizen Aug 21, 2023

12rambau Aug 21, 2023 •

edited

Loading

mariobuikhuizen Aug 21, 2023

mariobuikhuizen commented Aug 21, 2023

12rambau commented Aug 21, 2023

build: refactor build strategy #259

build: refactor build strategy #259

Conversation

12rambau commented Apr 21, 2023 • edited Loading

12rambau commented Apr 21, 2023 • edited Loading

maartenbreddels commented Apr 21, 2023

maartenbreddels commented Apr 25, 2023

12rambau commented Apr 25, 2023 • edited Loading

12rambau commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

12rambau commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

12rambau commented Apr 26, 2023

maartenbreddels commented Apr 26, 2023

maartenbreddels left a comment

Choose a reason for hiding this comment

12rambau commented Apr 26, 2023 • edited Loading

12rambau commented Apr 27, 2023

maartenbreddels commented Apr 27, 2023

12rambau commented Jun 13, 2023

maartenbreddels commented Jun 14, 2023

12rambau commented Jun 14, 2023

12rambau commented Jul 17, 2023

mariobuikhuizen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

12rambau Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariobuikhuizen commented Aug 21, 2023

12rambau commented Aug 21, 2023

12rambau commented Apr 21, 2023 •

edited

Loading

12rambau commented Apr 21, 2023 •

edited

Loading

12rambau commented Apr 25, 2023 •

edited

Loading

12rambau commented Apr 26, 2023 •

edited

Loading

12rambau Aug 21, 2023 •

edited

Loading