Rust/maturin CI

[alecandido]

• generate workflow with maturin generate-ci (and manually tune)
• write crates publishing workflow
  • list crates sorted by dependency
  • translate to Python
• dynamically resolve the version (git describe --tags)
• make workflows reusable (at least maturin) -> Fix maturin deployment to PyPI #384
• revert TODOs in workflows

[alecandido]

Unfortunately, to publish on crates.io path dependencies are not allowed, nor automatically resolved: you have to specify the version number of the dependencies.

Since there is also the workspace version to bump (that is the one inherited as version by all the other ones, but not possible to inherit as a dependency) I wrote a script to bump them all at the same time.

I don't like too much the result (because of irrelevant formatting), so I'd not commit it. But I was considering going for something like poetry-dynamic-versioning (though not as fancy) and avoid specifying a version at all in the repository (having some kind of placeholder, e.g. 0.0.0) and replacing it during the CI run.

Opinions on this? @felixhekhorn

---

PineAPPL has a similar script, doing string manipulation as only shells know...
https://github.com/NNPDF/pineappl/blob/05a115f2bd446aeeb5f01cbd5cfb3c4bb16213dc/maintainer/make-release.sh#L89-L98
but I prefer data structures, so for my convenience I've written the script in Nushell.

It's pretty nice, but in the hindsight I should have written it in Python 🤦‍♂️
In any case, it's working, and it's just a temporary tool to experiment the approach above. And I'm sure @Radonirinaunimi would be happy to convert it in Python, if needed :)

[alecandido]

I'm missing the crates publishing workflow, but the biggest part was the version one.

In Nushell (it's quite intuitive), it would look like the following:

# install Rust & Cargo
do { cd crates; nu bump-versions.nu }
ls crates | where type == dir | get name | filter {|n| $"($n)/Cargo.toml" | path exists } | each {|p| split row "/" | last} | each {|p| cargo publish -p $p }

It almost works, since it actually requires that they are listed in reverse-dependency order (first you have to upload the dependency, and then the dependant... makes sense...).
So, I will give up on the automated crates resolution, and just spell out the names in a file, in a sensible order (the only price is that whenever a new crate will be added, a line will have to be added to the file, in a manually resolved position... pretty cheap).

[alecandido]

A further caveat is that the maturin workflow is tuned to release a single package.
I'm assuming that we're not releasing more than a single Python package written in Rust, that essentially will bridge the two worlds.
However, at some point we might want to make expressions (ad and kernels) available once more in Python (maybe?). It will also be possible, and relatively simple, as soon as I'll make the workflow reusable, taking the path to the manifest as an input (it will simply have to be called multiple times).

@felixhekhorn this PR is still draft, since I haven't finished the crates workflow (it will be pretty quick), but (as usual) testing the workflows will be a pain (though making them simple, or generated by someone else, will reduce the attempts).
The unfortunate part is that:

• we will need to merge in master before knowing whether they are working (because otherwise I don't have access to the workflows)
• we will need to tag, to check the dynamical version resolution (so we will need a pre-release, possibly a few)

Sorry, I'd have liked to solve all in one PR, but GitHub in this respect is a bit annoying, and testing the CI is unpleasant in general (you don't want to test in production, but sooner or later you have to...).

[felixhekhorn]

Re version: I've no strong opinion - what ever works; even our current poetry setup has 0.0.0, but what I understood from #365 is that this can be updated now ...

Re version script: please let's not introduce another language - for testing this is fine, but looking at the script and never seeing the language before, I believe it should be simple enough to translate to Python, which I would definitely prefer

However, at some point we might want to make expressions (ad and kernels) available once more in Python (maybe?).

yes, I'd say eventually we maybe want the ad + ome also expose back to Python (the goal was in the first place to expose to more languages). Kernels (i.e. the linear algebra = matrix decomposition and stuff) are still in Python and I would currently not plan to move them (they are dealt by numpy). As for the crates: ekore will be independent and exposed, but eko(-)rs is tailored for eko (i.e. it is tightly bound to the quad_ker implementation, which surely is internal) so I'd say there is no need to expose it

Re base branch: why did you base this PR on #369 instead of master? this is completely decoupled since it is infrastructure related and not physics related - I would change the base branch and eventually rebase #369

Re merging: yes, sure if needed merge this - we all know CI testing is a pain in github. Can you please adjust the patches here (and below) so we can still hook the rust part in place?

[alecandido]

Re version: I've no strong opinion - what ever works; even our current poetry setup has 0.0.0, but what I understood from #365 is that this can be updated now ...

Nope, that will have 0.0.0 anyhow in the file in the repo, but the installation version will be updated even when installing in development (i.e. from a commit, or not even a commit, that is not a tag, just by appending the commit label, and the dirty keyword for uncommitted changes). So, it will be at the level of generated files.

Re version script: please let's not introduce another language - for testing this is fine, but looking at the script and never seeing the language before, I believe it should be simple enough to translate to Python, which I would definitely prefer

Shells are easier to compose commands :P
It was faster to iterate, but I knew I had to write it in Python...

As for the crates: ekore will be independent and exposed, but eko(-)rs is tailored for eko (i.e. it is tightly bound to the quad_ker implementation, which surely is internal) so I'd say there is no need to expose it

I would actually release it anyhow, it will make it easier to move things across the boundary

Re merging: yes, sure if needed merge this - we all know CI testing is a pain in github. Can you please adjust the patches here (and below) so we can still hook the rust part in place?

Sure, that's just a matter of removing crates/ in front of Cargo.toml and target/....

[alecandido]

Sorry, it seems that adding push: branches: events to workflows they are also able to run on branches, even when not present in the default branch.

I'm now testing them in this PR, and I will remove the unwanted events before merging. After that, we will need anyhow at least one pre-release to test them in "production mode", but hopefully not many of them (and avoid much debug in master)

[alecandido]

@felixhekhorn @cschwan I found how to share crates ownership with a GitHub team:

cargo owner --add github:nnpdf:theory

(to do this, you should cargo login with a crates.io token with granted change-owners permission)

We need it to be a team, so I'm going to use @NNPDF/theory for the time being.

I'm just waiting for the crates.io app access from one of the @NNPDF owners, and then it's a matter of a single command.

[alecandido]

Btw, docs generation is failing because of wrong location for the KaTeX header
https://docs.rs/crate/eko/0.1.1-alpha.1/builds/1225614

@felixhekhorn make sure that you can show them with cargo doc -p eko --open from top-level (or something similar).

[felixhekhorn]

@alecandido can you please have a look to fe8a8db ? Does this work? I suppressed the crates.yml to not create a broken state just in case

(PS: it technically works - but is this what we want?) Blub, it does not ... I need to fix that first

[felixhekhorn]

I did run the command in the shell and that worked and then I simply copied to poe assuming it works just as well, but no - he wants some special care 🙃

[alecandido]

I did run the command in the shell and that worked and then I simply copied to poe assuming it works just as well, but no - he wants some special care 🙃

I was about to write you the same, before seeing that you already fixed it (maybe you even received the notification)
http://mywiki.wooledge.org/BashFAQ/082

[alecandido]

@alecandido can you please have a look to fe8a8db ? Does this work? I suppressed the crates.yml to not create a broken state just in case

Yes, it is what I had in mind.

To be fair, since there should be no other use case for bump-versions.py, I was considering shelling out git describe --tags with a subprocess.run() within the script, to avoid complex handling somewhere else. But I was unsure myself.

[felixhekhorn]

Yes, it is what I had in mind.

To be fair, since there should be no other use case for bump-versions.py, I was considering shelling out git describe --tags with a subprocess.run() within the script, to avoid complex handling somewhere else. But I was unsure myself.

would have been also possible, but this way we can have languages separated (python + bash)

how to proceed with the deployment? should we push once - or we wait for the next tag? should I drop the TODO comments and just merge? (as is of right now)

[alecandido]

how to proceed with the deployment? should we push once - or we wait for the next tag? should I drop the TODO comments and just merge? (as is of right now)

Maybe just tag the commit on the branch, with a prerelease commit like those used until now (possibly deactivating all the other workflows running on tags).

would have been also possible, but this way we can have languages separated (python + bash)

True in general, but that's not even bash (it's just a single invocation of an external program, git), and it would make the script more self-contained.
I'm still thinking about:

• make workflows reusable (at least maturin)

since I may serve the script from https://github.com/NNPDF/workflows, instead of this repo.

[felixhekhorn]

Shall we merge this? even with broken CI ... I'd like to have the updated workflow and commands in master - else we should tick the again the people in your last comment 😇

[felixhekhorn]

Shall we merge this? even with broken CI ... I'd like to have the updated workflow and commands in master - else we should tick the again the people in your last comment 😇

as said I merge this now, since I'd like to have clippy in master; the left-over is tracked in #384 - the broken maturin deployment is (at the moment) not a showstopper and we can/should fix it at a later point