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

ci: Add doctests to CI again. #767

Merged
merged 33 commits into from
Dec 16, 2024
Merged

Conversation

hameerabbasi
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • πŸͺ„ Feature
  • 🐞 Bug Fix
  • πŸ”§ Optimization
  • πŸ“š Documentation
  • πŸ§ͺ Test
  • πŸ› οΈ Other

Related issues

  • Related issue #
  • Closes #

Checklist

  • Code follows style guide
  • Tests added
  • Documented the changes

Please explain your changes below.

Copy link

codspeed-hq bot commented Sep 3, 2024

CodSpeed Performance Report

Merging #767 will degrade performances by 41.43%

Comparing hameerabbasi:doctests-2 (e86fc3a) with main (b4ba7c6)

Summary

⚑ 1 improvements
❌ 1 regressions
βœ… 338 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main hameerabbasi:doctests-2 Change
⚑ test_index_fancy[side=100-rank=1-format='coo'] 1,242.2 ¡s 642.4 ¡s +93.36%
❌ test_index_slice[side=100-rank=2-format='gcxs'] 2.4 ms 4.1 ms -41.43%

@hameerabbasi
Copy link
Collaborator Author

@willow-ahrens Doctest issue.

@willow-ahrens
Copy link
Collaborator

using numba from the numba channel (with a statically linked llvmlite) fixes the segfault issue on mac. It appears that the segfault issue is mac-only at this point, as linux supports some special symbol resolution thing that fixed this problem.

Proposed solution: I would like to leave a warning to users on startup in the event that they have tried to load this particular config of packages. I think we could warn in pydata/sparse whenever we see (conda in use) AND (macos) AND (numba was not installed from numba channel)

As a side note: it is odd to me that we are loading both numba and finch at the same time. we might reduce some of this headache by switching between backends as a build option, rather than a runtime option.

@willow-ahrens
Copy link
Collaborator

I cannot get a reproducer for this in docker, though I have tried. If anyone can get a reproducer for this that would be great, then we can put it in an issue where we describe the problem. If you ssh into the CI runner, even doing "import sparse.finch_backend" will segfault, so we don't need all the pytest stuff to reproduce.

@willow-ahrens
Copy link
Collaborator

I've put a comment in to pyjuliapackage that we experience an issue with parallel installs, and suggested a fix here: JuliaPy/pyjuliapkg#19

@willow-ahrens
Copy link
Collaborator

also, note this comment regarding installation of juliacall and numba on conda: JuliaPy/PythonCall.jl#215 (comment)

Copy link
Collaborator

@mtsokol mtsokol left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question.

.github/workflows/ci.yml Show resolved Hide resolved
@mtsokol
Copy link
Collaborator

mtsokol commented Dec 16, 2024

And please remember about a commit squash!

@hameerabbasi hameerabbasi enabled auto-merge (squash) December 16, 2024 15:40
@hameerabbasi hameerabbasi merged commit 73c955f into pydata:main Dec 16, 2024
17 of 18 checks passed
@hameerabbasi hameerabbasi deleted the doctests-2 branch December 16, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants