Skip to content

fix: Python installation when running outside skbuild#335

Merged
lan496 merged 2 commits intospglib:developfrom
LecrisUT:fix/330
Feb 15, 2024
Merged

fix: Python installation when running outside skbuild#335
lan496 merged 2 commits intospglib:developfrom
LecrisUT:fix/330

Conversation

@LecrisUT
Copy link
Copy Markdown
Collaborator

@LecrisUT LecrisUT commented Sep 22, 2023 •
edited
Loading

  • Fixed the python installation
  • Refactored the test and execution so that installation is not required for doing pytest

Closes: #330 #344 #429
Depends on: #397

@LecrisUT LecrisUT force-pushed the fix/330 branch 5 times, most recently from 69f5f3a to 9eecab4 Compare September 22, 2023 17:13
@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Sep 22, 2023

Welp, 8d1ff02 has found a bunch of address sanitization issues 💀. It is also rather tricky to get the address sanitization working on the pytest themselves. I think #303 really needs to be implemented and move all of the tests into ctest. That way we can also minimize the number of builders.

@LecrisUT LecrisUT added bug help wanted Need help from contributors labels Sep 22, 2023
@lan496
Copy link
Copy Markdown
Member

lan496 commented Sep 22, 2023

I do not fully understand the context, but do we need address sanitizer for the Python interface?
It seems somewhat overwhelming for Spglib's Python developers to move Python tests into ctest.

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Sep 23, 2023

The core issue is not with the python interface having the address sanitizer, but that the C library tests do not cover all cases. That's why the main thing to do is #303 so we can move the main tests to ctest and not have the pytests run with sanitizers (at least not always)

@LecrisUT LecrisUT mentioned this pull request Oct 4, 2023
Closed
@LecrisUT LecrisUT mentioned this pull request Nov 13, 2023
Closed
@lan496 lan496 marked this pull request as draft January 7, 2024 04:52
@LecrisUT LecrisUT removed the help wanted Need help from contributors label Jan 25, 2024
@LecrisUT LecrisUT force-pushed the fix/330 branch 2 times, most recently from 6e1d018 to a44dc32 Compare January 25, 2024 17:56
@LecrisUT LecrisUT added this to the 2.4 milestone Jan 25, 2024
@LecrisUT LecrisUT marked this pull request as ready for review January 25, 2024 18:07
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (507291d) 83.59% compared to head (b21cb6f) 83.59%.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop     #335   +/-   ##
========================================
  Coverage    83.59%   83.59%           
========================================
  Files           24       24           
  Lines         8119     8119           
  Branches      1694     1687    -7     
========================================
  Hits          6787     6787           
  Misses        1332     1332
Flag Coverage Δ
c_api 74.44% <ø> (ø)
fortran_api 56.18% <ø> (ø)
python_api 79.74% <ø> (ø)
unit_tests 13.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LecrisUT LecrisUT mentioned this pull request Jan 25, 2024
Merged
1 task
@LecrisUT LecrisUT self-assigned this Jan 25, 2024
@LecrisUT LecrisUT linked an issue Jan 25, 2024 that may be closed by this pull request
CMake fails to find Python library for tests #344
Closed
@LecrisUT LecrisUT force-pushed the fix/330 branch 3 times, most recently from cba6edc to cab6084 Compare January 26, 2024 10:20
@lan496
Copy link
Copy Markdown
Member

lan496 commented Jan 27, 2024

@LecrisUT Please assign me when ready for review.

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 29, 2024

All 3 PRs are ready, but first we should wait on all of the packagings to catch up to find if everything is ok. We can track it better after #408.

After that we should review them in order, otherwise it is a bit much to navigate:
#397 -> #335 -> #308

@LecrisUT LecrisUT marked this pull request as draft February 5, 2024 23:26
@LecrisUT LecrisUT mentioned this pull request Feb 10, 2024
Merged
@LecrisUT LecrisUT marked this pull request as ready for review February 11, 2024 05:08
@LecrisUT LecrisUT linked an issue Feb 12, 2024 that may be closed by this pull request
Python interface is broken: ModuleNotFoundError: No module named 'spglib._version' #429
Closed
@LecrisUT LecrisUT requested a review from lan496 February 12, 2024 10:23
@LecrisUT LecrisUT assigned lan496 and unassigned LecrisUT Feb 12, 2024
@lan496
Copy link
Copy Markdown
Member

lan496 commented Feb 13, 2024

Let me review this PR next.

@LecrisUT
Fix python install path convention
a52cd0c 
- Added potentially missing _version.py file

Signed-off-by: Cristian Le <[email protected]>
@LecrisUT
Refactor ctest use of pytest
b21cb6f 
Signed-off-by: Cristian Le <[email protected]>
lan496
lan496 reviewed Feb 14, 2024
View reviewed changes
.github/workflows/step_test.yaml
- name: Install dependencies
run: |
python3 -m pip install pip-tools
pip-compile --extra=test --strip-extras pyproject.toml ${{ matrix.pre && '--pre' }}
Copy link
Copy Markdown
Member

@lan496 lan496 Feb 14, 2024

Choose a reason for hiding this comment

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

Why does this PR generate requirements.txt here? To lock package versions?

Copy link
Copy Markdown
Collaborator Author

@LecrisUT LecrisUT Feb 14, 2024

Choose a reason for hiding this comment

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

pip does not have a mode to install only dependencies, and that is the closest approach I've found to do that. What I wanted to be covered there is the case where the Spglib is built without scikit-build-core being involved. Weird though that it didn't error out in the previous iterations where I didn't include configure_file(_version.py.in spglib/_version.py)

Copy link
Copy Markdown
Member

@lan496 lan496 Feb 15, 2024

Choose a reason for hiding this comment

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

Although I feel tricky, it makes sense. Thanks!

Copy link
Copy Markdown
Collaborator Author

@LecrisUT LecrisUT Feb 15, 2024

Choose a reason for hiding this comment

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

I fully agree. Eventually I hope the skbuild-cli will be implemented, at which point we wouldn't need to care about such difference.

@lan496 lan496 merged commit a14e53e into spglib:develop Feb 15, 2024
@LecrisUT LecrisUT deleted the fix/330 branch February 15, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lan496 lan496 lan496 approved these changes

Assignees

@lan496 lan496

Labels

bug

Projects

Spglib developers' kanban
Status: ✅ Done

Milestone

2.4

2 participants

@LecrisUT @lan496