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 pip editable install with meson #39369

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Test pip editable install with meson #39369

wants to merge 19 commits into from
+20 −4

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Jan 22, 2025 •
edited
Loading

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
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 22, 2025 •
edited
Loading

Documentation preview for this PR (built with commit 13ec342; 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
This was referenced Feb 1, 2025
Merged
Merged
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
36a0d65 
    
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
7821f54 
    
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
264392e 
    
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 mentioned this pull request Feb 11, 2025
Closed
@user202729 user202729 mentioned this pull request Feb 11, 2025
Merged
5 tasks
@user202729 user202729 marked this pull request as draft February 11, 2025 17:14
This was referenced Feb 12, 2025
Merged
Merged
@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
07a4990 
    
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__
721c44c 
    
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
897e722 
    
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
98eec5d 
    
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
b7da8f2 
    
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__
a01815f 
    
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):
tobiasdiez
tobiasdiez approved these changes Feb 28, 2025
View reviewed changes
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 •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees
No one assigned
Labels
s: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@user202729 @kwankyu @tobiasdiez