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

Replace milksnake with maturin #2393

Merged
merged 19 commits into from
Dec 28, 2022
Merged

Replace milksnake with maturin #2393

merged 19 commits into from
Dec 28, 2022

Conversation

luizirber
Copy link
Member

@luizirber luizirber commented Dec 3, 2022

While investigating why the Python 3.11 PR failed in conda-forge I found out that there is an error in milksnake. Main issue is that milksnake seems to be abandoned, without recent releases, so submitting a PR to fix there might be unfruitful.

maturin is a PEP517-compatible backend for building packages and aimed at Rust packages (pure or mixed with more Python code, like sourmash). It is actively maintained, and supports the cffi bindings case that we use too.

This PR is based on pyocd/cmsis-pack-manager#195
Closes #1564 (previous try with setuptools-rust)

TODO

  • there is no support for setuptools_scm with maturin. Need to replace/version properly. turns out that setuptools_scm is doing something that PyPA doesn't recommend anymore. From core metadata and PEP 643:

    The fields Name and Version MUST NOT be marked as Dynamic.

  • Fix maturin develop/tox -e dev, currently failing.
  • Figure out how to pull Cargo.lock from workspace root. Maturin support for workspace is lacking, so the way I found to solve it (use the core crate Cargo.toml directly) expect the lock file to be src/core/Cargo.lock, but that doesn't exist.
  • Versioning. There is no support for dynamic version based on VCS tags (like setuptools_scm does), so we need to change the release process =/
    • Might be an opportunity to introduce release-please, example from rust-bio here with conventional commits checker here. This combination is nice because changing the PR title is enough to satisfy checks (and generate the changelog).

@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

Merging #2393 (8ce9ae7) into latest (c79e1ad) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           latest    #2393      +/-   ##
==========================================
- Coverage   84.09%   84.08%   -0.01%     
==========================================
  Files         130      130              
  Lines       15048    15044       -4     
  Branches     2208     2403     +195     
==========================================
- Hits        12654    12650       -4     
  Misses       2098     2098              
  Partials      296      296              
Flag Coverage Δ
python 92.22% <ø> (-0.01%) ⬇️
rust 57.72% <ø> (ø)

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

Impacted Files Coverage Δ
src/sourmash/cli/sbt_combine.py 100.00% <ø> (ø)
src/sourmash/cli/tax/metagenome.py 90.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@luizirber luizirber force-pushed the lirber/maturin branch 5 times, most recently from 0002a04 to 64afa31 Compare December 9, 2022 01:04
@messense
Copy link

Versioning. There is no support for dynamic version based on VCS tags (like setuptools_scm does), so we need to change the release process =/

FYI, something like this will also work as long as you release from CI:

@messense
Copy link

Figure out how to pull Cargo.lock from workspace root. Maturin support for workspace is lacking

Feel free to open an issue in maturin if it's better to be solved in upstream.

@luizirber luizirber force-pushed the lirber/maturin branch 4 times, most recently from 855b191 to 1505f9d Compare December 19, 2022 02:40
bors bot added a commit to PyO3/maturin that referenced this pull request Dec 19, 2022
1362: Add workspace lock file to sdist as a fallback r=messense a=messense

See sourmash-bio/sourmash#2393 (comment)

> Figure out how to pull `Cargo.lock` from workspace root.

cc `@luizirber`

Co-authored-by: messense <messense@icloud.com>
bors bot added a commit to PyO3/maturin that referenced this pull request Dec 19, 2022
1362: Add workspace lock file to sdist as a fallback r=messense a=messense

See sourmash-bio/sourmash#2393 (comment)

> Figure out how to pull `Cargo.lock` from workspace root.

cc `@luizirber`

Co-authored-by: messense <messense@icloud.com>
@luizirber luizirber force-pushed the lirber/maturin branch 8 times, most recently from 5ced5af to 1722e21 Compare December 24, 2022 22:34
@luizirber luizirber marked this pull request as ready for review December 24, 2022 23:22
@luizirber
Copy link
Member Author

The versioning is still pending, but I propose doing it in another PR because this one already have many moving parts...

ready for review @sourmash-bio/devs

@luizirber luizirber requested a review from a team December 24, 2022 23:24
Comment on lines 64 to 71
- name: tox cache
uses: actions/cache@v3
with:
path: .tox/
key: ${{ runner.os }}-tox-v3-${{ hashFiles('**/setup.cfg') }}
restore-keys: |
${{ runner.os }}-tox-v3-

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the tox cache due to tox 4 complaining about already existing env

@@ -268,26 +268,17 @@ jobs:

- uses: actions-rs/toolchain@v1
with:
toolchain: 1.48.0
toolchain: 1.56.1
Copy link
Member Author

Choose a reason for hiding this comment

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

rayon-core requires 1.56.1, so that's our MSRV now.

Copy link
Contributor

Choose a reason for hiding this comment

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

general query: is there a reason not to update the MSRV regularly anyway? It shouldn't matter for most things, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

general query: is there a reason not to update the MSRV regularly anyway? It shouldn't matter for most things, right?

Initially I was trying to follow LTS releases or what Debian supported, thinking that it would make it easier to package there.
For example, current Debian stable (bullseye) is 1.48: https://tracker.debian.org/pkg/rustc
(Ubuntu releases it in the security channel, so even 18.04 has Rust 1.61: https://packages.ubuntu.com/bionic/rustc)

But... there are no sourmash packages in either of them.

I don't think we should go updating MSRV every release, but 1.56.1 isa good one because it is the introduction of the 2021 edition.


- name: Check if it builds properly
uses: actions-rs/cargo@v1
with:
command: build
args: --all-features
Copy link
Member Author

Choose a reason for hiding this comment

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

make sure that we track optional features too, so dependabot won't leave us in a broken situation.

@@ -11,8 +11,7 @@
"html_dir": ".asv/html",
"build_cache_size": 8,
"build_command": [
"python -m pip install 'setuptools_scm[toml]>=4,<6' milksnake",
"python setup.py build",
"python -m pip install 'setuptools_scm[toml]>=4,<6' milksnake maturin",
Copy link
Member Author

Choose a reason for hiding this comment

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

still keeping setuptools and milksnake around for comparisons with older versions

@ctb
Copy link
Contributor

ctb commented Dec 27, 2022

Looks good! When you merge, can you create a new issue about updating the release process?

You could review & merge #2386 at the same time :)

@luizirber luizirber merged commit 2f2c692 into latest Dec 28, 2022
@luizirber luizirber deleted the lirber/maturin branch December 28, 2022 00:43
@luizirber
Copy link
Member Author

Looks good! When you merge, can you create a new issue about updating the release process?

-> #2425

@ctb ctb mentioned this pull request Mar 3, 2023
ctb added a commit that referenced this pull request Mar 3, 2023
# sourmash release 4.7.0

Major new features:

* provide an initial plugin architecture for sourmash that supports new
signature saving & loading mechanisms (#2428)
* add plugin support for new command-line subcommands (#2438)
* debias all containment values (#2243)

Minor new features:

* Use RankLineageInfo to simplify reading lineages (#2467)
* store taxids in lineageDB (#2466)
* Use new tax classes for taxonomic summarization (#2443)
* add tax summarization dataclasses for safety and flexibility (#2439)
* add `--scaled` to sourmash compare (#2414)
* replace `lca_utils.LineagePair` with `tax_utils.LineagePair` (#2441)
* Add new classes for lineage manipulation (#2437)

Cleanup and documentation updates:

* ReadTheDocs updates (#2445)
* update `sourmash compare` command-line docs (#2400)

Developer updates:

* fix python tests by bumping tox and pip cache versions (#2424)
* Update sphinx requirement from <6,>=4.4.0 to >=4.4.0,<7 (#2430)
* Build: replace milksnake with maturin (#2393)
* importlib_metadata is a dependency on old Python versions (#2484)
* Release docs: use two separate sed commands (#2483)
* minor fixes to release behavior (#2479)
* Use screed and maturin from nixpkgs in `flake.nix` (#2481)
* update release procedure after v4.6.0 and v4.6.1 (#2386)
* Update makefile and docs (#2432)

Dependabot updates:

* Bump once_cell from 1.17.0 to 1.17.1 (#2488)
* Bump ouroboros from 0.15.5 to 0.15.6 (#2487)
* Bump memmap2 from 0.5.8 to 0.5.9 (#2486)
* Bump supercharge/redis-github-action from 1.4.0 to 1.5.0 (#2485)
* Bump proptest from 1.0.0 to 1.1.0 (#2460)
* Bump web-sys from 0.3.60 to 0.3.61 (#2461)
* Bump serde_json from 1.0.91 to 1.0.93 (#2471)
* Bump wasm-bindgen-test from 0.3.33 to 0.3.34 (#2463)
* Bump cachix/install-nix-action from 18 to 19 (#2459)
* Bump wasm-bindgen from 0.2.83 to 0.2.84 (#2464)
* Bump typed-builder from 0.11.0 to 0.12.0 (#2451)
* Bump bumpalo from 3.9.1 to 3.12.0 (#2450)
* Bump pypa/cibuildwheel from 2.11.4 to 2.12.0 (#2447)
* Bump bzip2 from 0.4.3 to 0.4.4 (#2444)
* Bump once_cell from 1.14.0 to 1.17.0 (#2429)
* Bump serde from 1.0.151 to 1.0.152 (#2423)
* Bump pypa/cibuildwheel from 2.11.3 to 2.11.4 (#2422)
* Bump serde_json from 1.0.89 to 1.0.91 (#2418)
* Bump serde from 1.0.150 to 1.0.151 (#2419)
* Bump thiserror from 1.0.37 to 1.0.38 (#2417)
* Bump finch from 0.4.3 to 0.5.0 (#2416)
* Bump rayon from 1.6.0 to 1.6.1 (#2404)
* Bump serde from 1.0.149 to 1.0.150 (#2403)
* Bump pypa/cibuildwheel from 2.11.2 to 2.11.3 (#2402)
* Bump serde from 1.0.148 to 1.0.149 (#2397)
* Bump capnp from 0.14.5 to 0.14.11 (#2396)
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.

3 participants