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.

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

Test Python 3.9 and 3.12 on CI, test minimum dependencies #1029

Merged
merged 13 commits into from
Oct 15, 2024
Merged

Test Python 3.9 and 3.12 on CI, test minimum dependencies #1029

merged 13 commits into from
Oct 15, 2024

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Oct 11, 2024 •
edited
Loading

Related to multiple past issues where dependencies where misspecified.

@krassowski krassowski added the maintenance Change related to maintenance of the repository label Oct 11, 2024
@krassowski
Copy link
Member Author

Good, it caught the reported issue:

Screenshot from 2024-10-11 16-04-30

Bad, the custom install script is reinstalling the pip packages invalidating the resolution in base action. I will remove it.

@krassowski krassowski changed the title Test Python 3.9 and 3.13 on CI, test minimum dependencies Test Python 3.9 and 3.12 on CI, test minimum dependencies Oct 11, 2024
@krassowski krassowski mentioned this pull request Oct 11, 2024
Open
@krassowski krassowski marked this pull request as ready for review October 11, 2024 15:56
@krassowski
Copy link
Member Author

krassowski commented Oct 14, 2024 •
edited
Loading

Looking at the pip list output it does not actually install the minimum required versions in the minimum variant. I think this is likely either:

  • because the resolution rules do not apply to dependencies (as in metaproject dependencies i.e. jupyter-ai-magics and jupyter-ai) or
  • because in course of jlpm dev-install the resolutions get overwritten.

This PR could still be merged as-is just to ensure the Python versions are covered; I will iterate on the minimum version installation later either way.

@krassowski krassowski mentioned this pull request Oct 14, 2024
Open
@krassowski krassowski mentioned this pull request Oct 14, 2024
Open
dlqqq
dlqqq requested changes Oct 14, 2024
View reviewed changes
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@krassowski Thank you for opening this PR! This is a valuable contribution that will help prevent similar issues from arising in the future. I've left a few points of feedback for you to address below.

Looking at the pip list output it does not actually install the minimum required versions in the minimum variant.

I think to support dependency-type being used in GitHub job runs, we need to figure out how this is being passed to the shell command being run. For example, does this set an environment variable that scripts are supposed to use to determine how to install their dependencies? I can also investigate this myself later if you'd prefer my help on this.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
packages/jupyter-ai/jupyter_ai/document_loaders/directory.py Outdated Show resolved Hide resolved
@krassowski
Copy link
Member Author

To mention it explicitly, the issue with minimum dependency_type is being solved with jupyterlab/maintainer-tools#250.

I think to support dependency-type being used in GitHub job runs, we need to figure out how this is being passed to the shell command being run. For example, does this set an environment variable that scripts are supposed to use to determine how to install their dependencies?

dependency_type is an input to base-setup action (I personally would prefer if it was passed explicitly but I don't feel like trying to fix something that ain't broke). When it is set to minimum three things happen:

  • pip constraints are discovered from the extension source (how is an implementation detail) and written into a constraints.txt file
  • the contsraints are installed using pip -c option
  • the path to the constraints file is exported in PIP_CONSTRAINT environment variable (which is recognised by pip in any subsequent operations).

@krassowski
Copy link
Member Author

@dlqqq does jupyterlab/maintainer-tools#250 look good to you?

@krassowski
Update the requires-python to >=3.9,<3.13 in monorepo
9725225
@krassowski
Drop include_hidden=all_files from iglob instead
610eeee 
as per review users can still do `/learn .*` and
it would be in line how `bash`/`zsh` works (e.g.
`ls * -a` does not files in hidden directories
but `ls .*` does).
dlqqq
dlqqq approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@krassowski This PR looks great, thank you for contributing this! I've also reviewed jupyterlab/maintainer-tools#250.

@dlqqq dlqqq merged commit 41bccfe into jupyterlab:main Oct 15, 2024
10 checks passed
@dlqqq dlqqq mentioned this pull request Oct 15, 2024
Closed
Marchlak pushed a commit to Marchlak/jupyter-ai that referenced this pull request Oct 28, 2024
@krassowski @pre-commit-ci @Marchlak
Test Python 3.9 and 3.12 on CI, test minimum dependencies (jupyterlab…
0677737 
…#1029)

* Test Python 3.9 and 3.13 on CI, test minimum dependencies

* Bump versions in pyproject.toml files, define hatch build

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix spurious dev install in Python unit tests job

* Restore the install from script, add pip list

* Fix the issue

* Satisfy mypy typing, fix missing import

* Fail if any of commands in install script fails

* Revert back the attempt to use 3.13

* Add local dependencies specification

* Update the `requires-python` to `>=3.9,<3.13` in monorepo

* Drop `include_hidden=all_files` from `iglob` instead

as per review users can still do `/learn .*` and
it would be in line how `bash`/`zsh` works (e.g.
`ls * -a` does not files in hidden directories
but `ls .*` does).

* Update Python pin to just `>3.9`

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlqqq dlqqq dlqqq approved these changes

Assignees
No one assigned
Labels
maintenance Change related to maintenance of the repository
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@krassowski @dlqqq