Skip to content

Update num-modular, num-bigint, rand and glass_pumpkin. - #139

Open
plugwash wants to merge 1 commit into
uutils:mainfrom
plugwash:deps
Open

plugwash wants to merge 1 commit into
uutils:mainfrom
plugwash:deps

Conversation

@plugwash

@plugwash plugwash commented Oct 6, 2026 •

Copy link
Copy Markdown

This PR builds on https://github.com/plugwash/num-prime/pull/new/deps to further bump num-modular to 0.6.6 and update other dependencies to get a compatible set.

  • num-modular 0.6.6 has switched to num-bigint 0.5
  • num-bigint 0.5 supports rand 0.9 and 0.10 but not 0.8, use latest 0.10
  • glass_pumpkin (used only in benches) updated to 2.0.0-rc1 to support num-bigint 0.5

Closes #120
Closes #141

@sylvestre

Copy link
Copy Markdown
Contributor

sorry but it needs to be rebased

Comment thread Cargo.toml
num-bigint = { version = "0.5", features = ["rand_0_10"], optional = true }
num-integer = "0.1.46"
num-modular = "0.6.1"
num-modular = { version = "0.6.6", features = ["num-bigint"] }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this makes num-bigint a hard dependency even with --no-default-features, no?
could it stay behind big-int = ["dep:num-bigint", "num-modular/num-bigint"]?

Comment thread Cargo.toml
version = "0.4.6"
features = ["rand"]
optional = true
rand = "0.10"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rand 0.10 needs a much newer rustc than the rust-version = "1.70" above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What num-bigint does is define both rand_0_9 and rand_0_10 features. I was thinking we could do something similar if we don't bump the MSRV yet, since we want rand v0.10 for coreutils.

Comment thread Cargo.toml Resolved
Comment thread bench/Cargo.toml
criterion = { package = "codspeed-criterion-compat", version = "5.0.0" }
glass_pumpkin = "1.2.0"
num-bigint = { version = "0.4.6", features = ["rand"] }
glass_pumpkin = "2.0.0-rc1"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we really want to depend on a release candidate? even if it is just the bench :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems glass_pumpkin bumped to v2 after v1.10.0 was yanked due to the breaking change from upgrading rand_core. I've asked about the roadmap for v2 in LF-Decentralized-Trust-labs/agora-glass_pumpkin#44.

There are very few changes in v2 anyway, so it may be fine to depend on the RC for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

glass_pumpkin v2.0.0 was released.

Comment thread src/nt_funcs.rs Resolved
@plugwash

plugwash commented Oct 6, 2026

Copy link
Copy Markdown
Author

I was told to base my PR on #127 so that is what I did.....

@sylvestre

Copy link
Copy Markdown
Contributor

ok, i merged this PR but still conflicts

@xtqqczze

This comment was marked as resolved.

@xtqqczze

This comment was marked as resolved.

num-modular 0.6.6 has switched to num-bigint 0.5

num-bigint 0.5 supports rand 0.9 and 0.10 but not 0.8, use latest 0.10

glass_pumpkin (used only in benches) updated to 2.0.0-rc1 to support num-bigint 0.5
@plugwash

plugwash commented Oct 7, 2026

Copy link
Copy Markdown
Author

@plugwash As per #53, do you agree to dual-license your contribution under MIT OR Apache-2.0?

yes

@plugwash

plugwash commented Oct 7, 2026

Copy link
Copy Markdown
Author

I've rebased this onto main, dealt with the conflicts and moved the import in nt_funcs into the mod tests.

@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.07%. Comparing base (3f5db7d) to head (e51b5ee).
⚠️ Report is 87 commits behind head on main.

Files with missing lines Patch % Lines
examples/profile_factorization.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
+ Coverage   78.64%   86.07%   +7.43%     
==========================================
  Files          13       17       +4     
  Lines        2585     3972    +1387     
  Branches      230      264      +34     
==========================================
+ Hits         2033     3419    +1386     
+ Misses        552      550       -2     
- Partials        0        3       +3     
Flag Coverage Δ
macos_latest 85.97% <90.00%> (+8.33%) ⬆️
ubuntu_latest 85.67% <90.00%> (+7.49%) ⬆️
windows_latest 15.25% <0.00%> (+0.55%) ⬆️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed

codspeed Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will regress 4 benchmarks

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 7 improved benchmarks
❌ 4 regressed benchmarks
✅ 13 untouched benchmarks
⏩ 9 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ glass_pumpkin 2.1 ms 8.5 ms -75.93%
❌ glass_pumpkin 321.3 ms 470.1 ms -31.64%
❌ glass_pumpkin 12.5 µs 17.1 µs -26.77%
❌ glass_pumpkin (BPSW) 12.5 µs 16.2 µs -22.77%
⚡ glass_pumpkin (BPSW) 38.9 µs 12.6 µs ×3.1
⚡ glass_pumpkin 38.9 µs 13 µs ×3
⚡ glass_pumpkin 20.9 ms 8.7 ms ×2.4
⚡ glass_pumpkin (BPSW) 27.5 µs 15.9 µs +72.67%
⚡ glass_pumpkin 27.5 µs 16.5 µs +66.91%
⚡ glass_pumpkin (BPSW) 54.5 µs 39.9 µs +36.47%
⚡ glass_pumpkin 54.5 µs 40.4 µs +34.91%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing plugwash:deps (e51b5ee) with main (0f3dca7)

Open in CodSpeed

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@xtqqczze

xtqqczze commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

the package num-prime-bench depends on rand, with features: getrandom but rand does not have these features.

@plugwash

plugwash commented Oct 7, 2026

Copy link
Copy Markdown
Author

I'm at a loss, the tests passed locally and I don't see any mention of getrandom in bench/Cargo.toml.

@xtqqczze

xtqqczze commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Right, the error seems to be because these changes use rand 0.10, when the MSRV (1.70) does not support that. Ideally, we'd want to define both rand_0_9 and rand_0_10 features, but perhaps you could use rand 0.9 for now?

@sylvestre

Copy link
Copy Markdown
Contributor

clippy and rustfmt are sad :), and a bunch of jobs are failing

Comment thread src/nt_funcs.rs
use super::*;
use rand::{prelude::SliceRandom, random};
use std::iter::FromIterator;
use rand::prelude::IndexedRandom;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is SliceRandom still used? choose comes from IndexedRandom now.
please merge it into the use rand::{...} above

Comment thread src/rand.rs
macro_rules! impl_randprime_prim {
($($T:ty)*) => {$(
impl<R: Rng> RandPrime<$T> for R {
impl<R: RngExt> RandPrime<$T> for R {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this changes the public API (rand 0.10 rngs only, num-bigint 0.5 BigUint), so it is a breaking change.
we will need to bump the version to 0.7

@xtqqczze xtqqczze Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We already made a breaking change in #127, so yes will need to bump to 0.7.

@sylvestre

Copy link
Copy Markdown
Contributor

clippy is sad on 2 jobs :)

rustfmt is sad too :)
could you please run cargo fmt?

Diff in /home/runner/work/num-prime/num-prime/bench/benches/bench.rs:4:

 use criterion::{Criterion, SamplingMode};
 use glass_pumpkin::{prime as gprime, safe_prime as safe_gprime};
-use num_bigint::BigUint;
 use num_bigint::BigRng010;
+use num_bigint::BigUint;
 use num_prime::{nt_funcs, RandPrime};
 use rand::rngs::StdRng;
 use rand::SeedableRng;

@xtqqczze

Copy link
Copy Markdown
Contributor

@plugwash Are you planning to continue with this? If not, I’d be happy to take it over.

@plugwash

Copy link
Copy Markdown
Author

Feel free to take it over.

@xtqqczze

Copy link
Copy Markdown
Contributor

I've started working on this, but I would like to get #143 merged first.

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.

Upgrade to num-bigint 0.5

3 participants