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

Upgrade extension for JLab 4 #263

Merged
merged 20 commits into from
Aug 3, 2023
Merged

Conversation

mahendrapaipuri
Copy link
Contributor

@mahendrapaipuri mahendrapaipuri commented Jun 15, 2023

  • This PR upgrades the node dependencies of @jupyterlab to 4.0.0 and @lumino to 2.0.0.
  • Minor corrections in index.ts are made to work with JLab 4
  • Migrate to hatch build system which is now recommended build system by Jupyter project.
  • Migrate from versioneer to nodejs versioning. Python package will take the version string from package.json. Bumping of versions can be done using hatch version command.
  • Added black and pytest config to pyproject.toml
  • Use JLab 4 as build and run time dependencies
  • Update min Python version to 3.8 inline with JLab 4
  • Only JLab 4 is included in CI test cases. Python versions are also bumped to 3.8 and 3.11 in CI
  • Use actions from jupyterlab-maintainer-tools that does the setting up of Python and nodejs

Maintainers - If you prefer to have versioneer, let me know. I will add it back. The thing is versioneer is not very well integrated with pyproject.toml. There is hatch-vcs plugin that is made for hatch build system that does exactly same thing as versioneer but with less config burden.

@mahendrapaipuri mahendrapaipuri marked this pull request as draft June 15, 2023 11:43
@mahendrapaipuri mahendrapaipuri marked this pull request as ready for review June 15, 2023 12:05
@mahendrapaipuri mahendrapaipuri changed the title Upgrade node dependencies and extend test cases for JLab 3 and 4 Upgrade extension for JLab 4 Jun 17, 2023
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks you so much for raising this.

Did you follow the migration guide to generate this? I've run the tool myself and it seems to have made many more changes than are included in this PR. However I also can't get things to install correctly after running the tool.

@mahendrapaipuri
Copy link
Contributor Author

@jacobtomlinson Not really. I just bumped the node deps manually to 4.0.0. The script that you mentioned will migrate the extension to hatch build system which is recommended way to build extensions. Folks at Jupyter developed a hatch-jupyter-builder plugin for the hatch build system which avoids all the custom logic in the setup.py file.

If you want, I can extend this PR to hatch migration. We have already done it for our custom extensions and we have a very positive experience.

@jacobtomlinson
Copy link
Member

If you're happy to take that on that would be fantastic.

@mahendrapaipuri
Copy link
Contributor Author

@jacobtomlinson Please check the updated PR description!

@ian-r-rose
Copy link
Collaborator

@mahendrapaipuri thank you so much for this, and apologies for the slow review! I'm prioritizing this to get it over the line, and will have some more detailed comments this evening.

We just published a patch release targeting JupyterLab 3, and there are a few conflicts to resolve. I'm happy to do that unless you get to it faster.

I'm very excited to see versioneer and the setup.py go away :)

@mahendrapaipuri
Copy link
Contributor Author

mahendrapaipuri commented Aug 1, 2023

@ian-r-rose You are welcome. I dont see any huge problems due to conflicts that are raised due to #267. Most of the conflicting files (MANIFEST.in, yarn.lock and setup.py) will be obsolete and I will update the package.json will the new packages that you added in the #267.

I will wait for your comments before I resolve the conflicts.

Copy link
Collaborator

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Thanks @mahendrapaipuri. I'm still digesting the hatch-based changes, but had a few questions/comments

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.yarnrc.yml Show resolved Hide resolved
dask_labextension/_version.py Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Overall, this looks really great, my comments and questions are all pretty minor. Thank you @mahendrapaipuri!

pyproject.toml Outdated Show resolved Hide resolved
Update editor model in injectCode function

Upgrade to jlab 4, lumino 2 and yarn 3

Use jl>3 as build dep to support jl 3 and 4

Include .yarnrc in VCS and add .yarn in gitignore
Test on jl 3 and 4 and py 3.8 and 3.11

Should cover most of the cases
Update classifiers and min pyver to 3.8
Remove deprecated serverextension subcommand from build tests
Migrate from versioneer to nodejs versioning. The package
will be versioned based on version in package.json.
More details https://hatch.pypa.io/latest/version/

Removed obselete versioneer files and references

Removed contents of setup.py and unnecessary file MANIFEST.in

setup.cfg is retained as pyproject does not support flake8 config

Added black and pytest config to pyproject.toml
Seems like flake8 does not support inline comments anymore
Get rid of setup.cfg which is obsolete for the package
Fix minor bugs in pyproject config
Add instructions to JLab 4 compatible installation

Update instructions for release using hatch
@mahendrapaipuri
Copy link
Contributor Author

@ian-r-rose Addressed your PR comments, rebased onto new main and updated README too.

Copy link
Collaborator

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Thanks @mahendrapaipuri! One small suggestion, which I could do in a follow-up if you feel like wrapping this one up.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
Copy link
Collaborator

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Thanks again for all your work on this! I'm going to try to get a release out today

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.

3 participants