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

Test pip editable install with meson #39369

Merged
merged 20 commits into from
Mar 9, 2025

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Jan 22, 2025

Tests pip editable install with meson on GitHub Actions.

This uncovers several failing tests, which are fixed by (see dependencies below)

Note: review is hard because of the large number of dependencies. It is probably easier to look only in .github folder.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.

⌛ Dependencies

@user202729 user202729 marked this pull request as draft January 22, 2025 16:11
Copy link

github-actions bot commented Jan 22, 2025

Documentation preview for this PR (built with commit 43142c7; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729 user202729 marked this pull request as ready for review February 1, 2025 10:37
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39424: Fix bug with sage_getfile in meson editable install
    
Previously, the following test fails with meson editable install

```
2025-01-28T04:27:13.4765580Z File "src/sage/misc/cachefunc.pyx", line
881, in sage.misc.cachefunc.CachedFunction._instancedoc_
2025-01-28T04:27:13.4766297Z Failed example:
2025-01-28T04:27:13.4766651Z
os.path.exists(sage_getfile(I.groebner_basis))
2025-01-28T04:27:13.4767084Z Expected:
2025-01-28T04:27:13.4767330Z     True
2025-01-28T04:27:13.4767571Z Got:
2025-01-28T04:27:13.4767808Z     False
```

See https://github.com/sagemath/sage/actions/runs/13003203795/job/362655
39648 .

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.  (can't really test,
but see sagemath#39369)
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39424
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39424: Fix bug with sage_getfile in meson editable install
    
Previously, the following test fails with meson editable install

```
2025-01-28T04:27:13.4765580Z File "src/sage/misc/cachefunc.pyx", line
881, in sage.misc.cachefunc.CachedFunction._instancedoc_
2025-01-28T04:27:13.4766297Z Failed example:
2025-01-28T04:27:13.4766651Z
os.path.exists(sage_getfile(I.groebner_basis))
2025-01-28T04:27:13.4767084Z Expected:
2025-01-28T04:27:13.4767330Z     True
2025-01-28T04:27:13.4767571Z Got:
2025-01-28T04:27:13.4767808Z     False
```

See https://github.com/sagemath/sage/actions/runs/13003203795/job/362655
39648 .

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.  (can't really test,
but see sagemath#39369)
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39424
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 10, 2025
sagemathgh-39424: Fix bug with sage_getfile in meson editable install
    
Previously, the following test fails with meson editable install

```
2025-01-28T04:27:13.4765580Z File "src/sage/misc/cachefunc.pyx", line
881, in sage.misc.cachefunc.CachedFunction._instancedoc_
2025-01-28T04:27:13.4766297Z Failed example:
2025-01-28T04:27:13.4766651Z
os.path.exists(sage_getfile(I.groebner_basis))
2025-01-28T04:27:13.4767084Z Expected:
2025-01-28T04:27:13.4767330Z     True
2025-01-28T04:27:13.4767571Z Got:
2025-01-28T04:27:13.4767808Z     False
```

See https://github.com/sagemath/sage/actions/runs/13003203795/job/362655
39648 .

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.  (can't really test,
but see sagemath#39369)
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39424
Reported by: user202729
Reviewer(s): Tobias Diez
@user202729 user202729 marked this pull request as ready for review February 12, 2025 06:33
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2025
sagemathgh-39423: Use import_module instead of find_spec
    
Otherwise the test would fail with meson editable install. See https://g
ithub.com/sagemath/sage/actions/runs/13003203795/job/36265539648 .

Looks like the function was last changed in sagemath#36407. There was no
discussion why the simple implementation is not used.

This is part of the fix for this test. The other part needed is
sagemath#39498


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.  (can't really test,
but see sagemath#39369)
- [ ] I have updated the documentation and checked the documentation
preview.  (no documentation change)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39423
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2025
sagemathgh-39499: Improve sage_getfile by looking at __init__
    
Otherwise it will fail with meson editable install on objects like `x`
of type `Expression`, where the class does not have a docstring but
`__init__` method have.

The strategy is similar to `sage_getsourcelines` where `__init__` method
is looked in.

I choose to implement this instead of the more complex workaround (see
the comment below).

This fixes the test that fails in meson editable mode:

```
2025-02-11T20:13:36.4801998Z
**********************************************************************
2025-02-11T20:13:36.4803076Z File "src/sage/misc/sageinspect.py", line
1363, in sage.misc.sageinspect.sage_getfile_relative
2025-02-11T20:13:36.4804104Z Failed example:
2025-02-11T20:13:36.4804750Z     sage_getfile_relative(x)
# needs sage.symbolic
2025-02-11T20:13:36.4808395Z Expected:
2025-02-11T20:13:36.4813350Z     'sage/symbolic/expression.pyx'
2025-02-11T20:13:36.4814244Z Got:
2025-02-11T20:13:36.4815714Z
'/home/runner/work/sage/sage/builddir/src/sage/symbolic/expression.pyx'
2025-02-11T20:13:44.3128543Z
**********************************************************************
```

This may still fail in another case where neither class nor `__init__`
method has a docstring (in that case `?` will fail to get the file in
meson editable mode), but that's not in scope I guess. (I left a comment
there to explain)

See also sagemath#39369

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview. (change should be invisible to users not using meson editable
so…)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39499
Reported by: user202729
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 21, 2025
sagemathgh-39423: Use import_module instead of find_spec
    
Otherwise the test would fail with meson editable install. See https://g
ithub.com/sagemath/sage/actions/runs/13003203795/job/36265539648 .

Looks like the function was last changed in sagemath#36407. There was no
discussion why the simple implementation is not used.

This is part of the fix for this test. The other part needed is
sagemath#39498


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.  (can't really test,
but see sagemath#39369)
- [ ] I have updated the documentation and checked the documentation
preview.  (no documentation change)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39423
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 21, 2025
sagemathgh-39494: Fix more doctests in meson_editable install
    
Fix more bugs uncovered by sagemath#39369 .

Add a feature flag `meson_editable`, and put doctests behind this flag
accordingly.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39494
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 21, 2025
sagemathgh-39498: Apply sort and filter of walk_packages consistently
    
Previously, the sorting and filtering is only applied in case of
FileFinder. This makes it consistently applied.

Needed to make tests in sagemath#39369
pass.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39498
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 21, 2025
sagemathgh-39499: Improve sage_getfile by looking at __init__
    
Otherwise it will fail with meson editable install on objects like `x`
of type `Expression`, where the class does not have a docstring but
`__init__` method have.

The strategy is similar to `sage_getsourcelines` where `__init__` method
is looked in.

I choose to implement this instead of the more complex workaround (see
the comment below).

This fixes the test that fails in meson editable mode:

```
2025-02-11T20:13:36.4801998Z
**********************************************************************
2025-02-11T20:13:36.4803076Z File "src/sage/misc/sageinspect.py", line
1363, in sage.misc.sageinspect.sage_getfile_relative
2025-02-11T20:13:36.4804104Z Failed example:
2025-02-11T20:13:36.4804750Z     sage_getfile_relative(x)
# needs sage.symbolic
2025-02-11T20:13:36.4808395Z Expected:
2025-02-11T20:13:36.4813350Z     'sage/symbolic/expression.pyx'
2025-02-11T20:13:36.4814244Z Got:
2025-02-11T20:13:36.4815714Z
'/home/runner/work/sage/sage/builddir/src/sage/symbolic/expression.pyx'
2025-02-11T20:13:44.3128543Z
**********************************************************************
```

This may still fail in another case where neither class nor `__init__`
method has a docstring (in that case `?` will fail to get the file in
meson editable mode), but that's not in scope I guess. (I left a comment
there to explain)

See also sagemath#39369

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview. (change should be invisible to users not using meson editable
so…)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39499
Reported by: user202729
Reviewer(s):
Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I'm fine with this. @kwankyu any opinions?

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 1, 2025

No.

@user202729
Copy link
Contributor Author

We may also do something like "only test Python 3.11 non-editable & Python 3.12 editable" to save some carbon. Thoughts? (the old Conda matrix does something similar.)

@tobiasdiez
Copy link
Contributor

We may also do something like "only test Python 3.11 non-editable & Python 3.12 editable" to save some carbon. Thoughts? (the old Conda matrix does something similar.)

I like this! Having only one "editable" run should be sufficient, right? (github workflows have the include or so keyword to add a single config to a matrix)

@user202729
Copy link
Contributor Author

user202729 commented Mar 3, 2025

Okay done (use the trick first used in https://github.com/sagemath/sage/pull/39369/files )

runs-on: ${{ matrix.os }}-latest

strategy:
fail-fast: false
matrix:
os: [ubuntu]
python: ['3.11', '3.12']
editable: [false, true]
Copy link
Contributor

Choose a reason for hiding this comment

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

This now makes it hard to extend the matrix by other OS for PRs (where you would like to run all non-editable versions for all pythons and all systems).

Does the following work?

editable: 
- false
- ${{ github.event_name != 'pull_request' }} # PR: false (which is then ignored), Push: true

include: 
          - os: ubuntu
            python: 3.12
            editable: true # One additional editable run for PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what combination you are trying to do. The way it currently works is

in PR, only run the combinations listed below; otherwise, run all combinations

As long as in non-PR you want to run all combinations you can just edit the matrix. Hopefully you don't need to run too many combinations in pull request that you can afford listing each configuration explicitly.

So for example if you were to add windows and mac to the os: [ubuntu], then you could modify the list below to e.g.

        include:
          - os: ubuntu
            python: 3.11
            editable: false
          - os: mac
            python: 3.12
            editable: false
          - os: windows
            python: 3.12
            editable: true

or something like that. (which is probably sufficient right? Each factor got tested at least once)


In your suggestion, on push all combinations will be run, and on pull request 3 combinations will be run (which also work, I think).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully you don't need to run too many combinations in pull request that you can afford listing each configuration explicitly.

That's the problem. Once we add macos and windows to the matrix, it will run 3 (os) x 3 (python) systems for each PR (with proper caching this hopefully isn't too much total CI time). I prefer not to specify all of these combinations explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think one problem is do you really want to run 10 per PR. It seems like 3 is enough (see my suggestion above).

Although… I think if it gets sufficiently complicated it might be better to use full-blown code to generate the include list. (Does whatever language that GitHub Actions use have a for loop or list comprehension or map etc.? Seems not.)

e.g. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/running-variations-of-jobs-in-a-workflow#example-using-an-output-to-define-two-matrices could be used. But then this implies we need a separate script written in something like Python (is this preinstalled?) to generate the JSON.

Is it worth it or is this overengineering?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth it or is this overengineering?

It's a tad too complicated in my opinion.

I think one problem is do you really want to run 10 per PR. It seems like 3 is enough

In my opinion, issues should be caught at the PR level. And the only way to observe a bug, say on MacOS with Python 3.11, is to run exactly that combination. Preferably, it would run only once for the merge and not at every push to a PR but that's not possible with our current setup.

But anyway, for now I would prefer a solution that adds exactly one "editable" run for PRs (and perhaps for all Python versions on push to develop).

@user202729 user202729 force-pushed the meson-try-editable branch 2 times, most recently from bfe32de to 65dbc53 Compare March 8, 2025 08:49
@user202729 user202729 force-pushed the meson-try-editable branch from 65dbc53 to 43142c7 Compare March 8, 2025 08:51
Copy link
Contributor

@tobiasdiez tobiasdiez 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 your fixes of the editable mode!

@vbraun vbraun merged commit 2b842ce into sagemath:develop Mar 9, 2025
22 of 24 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.

4 participants