Skip to content

Conversation

@neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented Jul 30, 2025

This allows building the package from source with arbitrary build frontends, not just pip.
For instance, this allows:

uv build

or

python -m build

having pip as a build-time dependency is unsual and could easily be avoided by moving the information currently contained in requirements.txt and requirements-dev.txt to pyproject.toml, but that would also break any existing development workflows explicitly relying on their presence, so I consider it out of scope for now, but please let me know if you'd like me to do it too, either here or as a follow up PR.
I could also add explicit support for Python 3.12 and 3.13, but again, I'll treat it as out of scope unless explicitly agreed on first.

@neutrinoceros neutrinoceros force-pushed the bld/explicit-build-dependencies branch 3 times, most recently from 3d16cae to 6347c97 Compare July 30, 2025 16:25
@neutrinoceros neutrinoceros changed the title BLD: move static metadata to \pyproject.toml\, explicitly specify build backend and requirements BLD: move static metadata to `pyproject.tomlù and explicitly specify build backend and requirements Jul 30, 2025
@neutrinoceros neutrinoceros changed the title BLD: move static metadata to `pyproject.tomlù and explicitly specify build backend and requirements BLD: move static metadata to pyproject.toml and explicitly specify build backend and requirements Jul 30, 2025
@neutrinoceros neutrinoceros force-pushed the bld/explicit-build-dependencies branch 2 times, most recently from 27325ae to c6972e1 Compare July 30, 2025 16:33
@@ -0,0 +1,33 @@
[build-system]
build-backend = "setuptools.build_meta"
requires = [ "pip", "setuptools>=77" ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: I've used setuptools>=77 here to get support for PEP 639 license specification (project.license-files), but unfortunately this version of setuptools isn't available for Python 3.8. Would it be okay to also drop support for this end-of-life version of Python too ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in support of dropping it but Valentin should make the final call

@IainHammond
Copy link
Contributor

IainHammond commented Jul 30, 2025

Support for 3.13 is fine, I've been using it extensively with VIP for a few months without any problems

@neutrinoceros
Copy link
Contributor Author

yeah, I only noticed it wasn't explicitly tested, but I didn't hit any issue with it

@neutrinoceros
Copy link
Contributor Author

Just noticing now that #653 already existed but seems to have stalled.
Happy to add @Sand-jrd as a co-author on my commits if it helps moving this forward

@Sand-jrd
Copy link
Contributor

Sand-jrd commented Jul 30, 2025

Hello, thanks for bringing this up. Well it's fine no need to mention me just ignore my pull request. I'll close it later

@neutrinoceros neutrinoceros force-pushed the bld/explicit-build-dependencies branch 5 times, most recently from 91d5113 to b18e4b8 Compare July 31, 2025 13:20
@neutrinoceros
Copy link
Contributor Author

Whoops, I see that CI was manually run but an import cycle failed it. It should be fixed now. Although, please note I still expect it to fail on Python 3.8 (but only on this version). See #670 (comment)

@neutrinoceros
Copy link
Contributor Author

Fun fact: Python 3.7 was dropped exactly 2 years ago today in #610
Given the precedent, I'm hoping that dropping 3.8 now isn't controversial

@neutrinoceros
Copy link
Contributor Author

Oh, interesting, apparently CI doesn't catch the known issue with building on Python 3.8 because it never actually builds the package and instead runs against the source tree. This is also easy to fix, but requires a possibly disrupting change of the repository layout from flat to src. See this doc if this terminology is unfamiliar: https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout

@VChristiaens
Copy link
Contributor

Hi @neutrinoceros - thanks so much for your PRs! As you guessed, I didn't find the time to sit down and do this properly lately, even though @Sand-jrd broke the back of the work in her PR. Conflicts inserted themselves in that PR due to accepting other PRs in the meantime... and I didn't find the time to solve them lately so the PR sort of stalled. Your help is very timely to push things forward!
First, to answer your questions: I'm totally fine with dropping Python 3.8 compatibility. I also agree it's time to add 3.11, 3.12 and 3.13 support. Next thing would be to add these versions to the test suite.
Next, before accepting your PR I have a couple of questions, to know whether it fully matches what I had envisioned:

  • would each future version upgrade involve manually adapting the version mentioned in the pyproject.toml file? If not, is there a way to have it tied to new published releases? Unless, this is not the recommended approach?
  • currently the CI is set to rely on installing the dependences mentioned in the requirements-dev.txt file to create the environment to run the test suite, how would you suggest to proceed in absence of this text file?
  • I'd be fine with moving to a flat layout, could your suggestion also be included in the PR to be tested?
    Thanks again for your help - this is very kind!

@neutrinoceros
Copy link
Contributor Author

would each future version upgrade involve manually adapting the version mentioned in the pyproject.toml file? If not, is there a way to have it tied to new published releases? Unless, this is not the recommended approach?

That's a totally valid approach, but in case you want to get fancy, you could also use CLIs to bump it for you
(for instance, I use uv version --bump minor), or you could also use setuptools-scm (as previously proposed in #653) to get the version metadata compiled at build-time from a git tag.

currently the CI is set to rely on installing the dependences mentioned in the requirements-dev.txt file to create the environment to run the test suite, how would you suggest to proceed in absence of this text file?

See #671 for this point

I'd be fine with moving to a flat layout, could your suggestion also be included in the PR to be tested?

If you don't mind I'd prefer keeping a reasonably small scope for this PR, especially because, as a first time contributor, my patches currently need manual approval before CI can be run, and I have a lot more where that comes from so I figure it might save everyone some time if I could open multiple PRs at once and have CI run in parallel. Your call !

@neutrinoceros
Copy link
Contributor Author

I see a test already failed

tests/pre_3_10/test_dataset.py F

Unfortunately the same test seems to pass locally (tried on 2 different machines) so I'll just wait for CI to complete to see what's wrong. I also note that I couldn't run the entire test suite on either of my machines as it seems prohibitively costly (and I think I'm seeing some issues with parallelism)

@neutrinoceros
Copy link
Contributor Author

Hum, I think CI might actually have gotten stuck, and I have serious doubts that it could be related to my PR (instead, I suspect there might have been a change in the environment since the last successful run). Would it be possible to launch a control run against the master branch and see if it completes there ?

@IainHammond
Copy link
Contributor

Test 1 always takes the longest. Usually about 1h30m to 2h

@neutrinoceros
Copy link
Contributor Author

nevermind I got one to finish and I see now that the failure is also a symptom of a problem I already hinted at: CI is testing against the source tree, not a built package. Anyway I'll just work around it for this fundamental PR and will fix the workaround later on !

@neutrinoceros neutrinoceros force-pushed the bld/explicit-build-dependencies branch from b18e4b8 to 120b908 Compare July 31, 2025 16:49
@neutrinoceros neutrinoceros force-pushed the bld/explicit-build-dependencies branch from 120b908 to 6a97aa5 Compare July 31, 2025 16:53
@neutrinoceros
Copy link
Contributor Author

CI finally looks good, thank you for your patience !
note: I might be able to tackle a couple of issues I identified this very afternoon if this one is merged soon enough (but of course, no big deal if it isn't).

@VChristiaens VChristiaens merged commit 2d65a29 into vortex-exoplanet:master Aug 1, 2025
14 checks passed
@VChristiaens
Copy link
Contributor

Fantastic! Thanks a lot @neutrinoceros!!

@neutrinoceros neutrinoceros deleted the bld/explicit-build-dependencies branch August 1, 2025 11:57
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