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

Add ability to customize max default SizeRange via env var. #338

Merged
merged 3 commits into from
Jan 13, 2024

Conversation

nipunn1313
Copy link
Contributor

The implementation directly accesses Config::default() from SizeRange::default().

I don't love the implementation for two reasons

  • It clones the full config on every call to SizeRange::default()
  • It is hard to test.

However, it does achieve the goal. I am open to better ideas!

We could avoid the clone by changing the behavior of Config::default() (perhaps (&Config)::default() - or by making DEFAULT_CONFIG lazystatic public.

Fixes #320

After implementing this, I am starting to feel more partial to the alternate approach of reducing the default max sizerange drastically to 0..8 and making people increase it manually via a non-default strategy - if that's the behavior they want. The perf of nested vecs snowballs out of control quite fast IMO and a bit of a surprise.

The implementation directly accesses Config::default() from
SizeRange::default().

I don't love the implementation for two reasons
- It clones the full config on every call to SizeRange::default()
- It is hard to test.

However, it does achieve the goal. I am open to better ideas!
@tzemanovic
Copy link
Contributor

Thanks! Personally, I'd be in favor of changing the behavior of Config::default(). We could just remove the static ref DEFAULT_CONFIG that calls contextualize_config to resolve env vars and instead call contextualize_config from TestRunner::new to preserve the behavior.

When using proptest! it will already call contextualize_config - I've added this in #318 to allow overriding non-default config and it's also a bit sub-optimal because the env vars are being resolved twice. With the proposed change to TestRunner::new we could undo the changes in sugar.rs (https://github.com/proptest-rs/proptest/pull/318/files#diff-80b8c1ddfc4dfa65d2e43d283c01f08dc1b47e77b79a69ae9a1e947894488b70).

I would also not oppose reducing the default.

@tzemanovic
Copy link
Contributor

I've added the changes I proposed here https://github.com/nipunn1313/proptest/compare/nipunn/proptest_max_size...tzemanovic:proptest:nipunn/proptest_max_size_2?expand=1. Please lmk if you're happy with the changes and I can push them back here

@matthew-russo
Copy link
Member

I'll give this a look this evening after work. If it's reusing existing config mechanisms rather than plunking env vars in the impl of Default im more in favor

@matthew-russo
Copy link
Member

matthew-russo commented Jul 30, 2023

Agh I never published my comment. I didn't realize what the original proposal was. I'm fine with the original PR opened here. Tomas for your alternative changes, I'd want the default impl of size range to use the config contextualization rather than directly reading env vars. That way we isolate all env-based configuration to just Config.

Whats the rationale behind removing the contextualization from Config::Default?

@tzemanovic
Copy link
Contributor

@matthew-russo My main motivation is to remove the cloning from impl Default for Config. The other options @nipunn1313 suggested were to either:

  • impl Default for &Config where for no-std were we'd need to add another static be able to impl this as there is non-const rng_algorithm: RngAlgorithm::default() used in fn default_default_config
  • make static ref DEFAULT_CONFIG pub, but this is also not available in no-std

@matthew-russo
Copy link
Member

If this is perf related are there any numbers showing removing the clones improves test runtime? I'd be pretty surprised if the config generation is where times being spent. I haven't spent much time with this area of the code though.

This would change the behavior of existing users which I'm skeptical of doing without a breaking version change.

@tzemanovic
Copy link
Contributor

If this is perf related are there any numbers showing removing the clones improves test runtime? I'd be pretty surprised if the config generation is where times being spent. I haven't spent much time with this area of the code though.

This would change the behavior of existing users which I'm skeptical of doing without a breaking version change.

On my machine it is slightly better than the previous impl that was cloning the whole Config just to get this single field, though I agree that this is not likely a bottleneck of any test.

The change itself however is not externally a breaking change. Even though the behavior of impl Default for Config is changed, the only places where the Config appears in public API is TestRunner::new/new_with_rng which with the added call to contextualize_config inside them remain the same. The only difference would be if someone were to use Config::default() without the proptest! macro, manually override some field on it while at the same time having an env var also overriding this field. Previously, the first override would take precedence, now it would be the second override from an env var. I'm not sure if I'd go as far as call the current precedence order a bug and I doubt anyone is relying on it, but I think the new order makes more sense anyway.

@rex-remind101
Copy link
Collaborator

I took a look over this myself and just wanted to weigh in with my opinion

  • This pr to me seems good as-is and I'm fine merging it.
  • The default of 100 has been a nuisance to me before, however I'd consider it a breaking change to change our default cardinality.

I read through @tzemanovic 's pr and though I agree with some of the motivation I think there's some things to consider

I'd want the default impl of size range to use the config contextualization rather than directly reading env vars. That way we isolate all env-based configuration to just Config.

  • I agree with @matthew-russo here, this line doesn't follow the preexisting pattern the rest of the codebase seems to use by accessing Config::default() directly, moving away from that pattern seems like a larger change to me outside of scope of this pr.
  • However, "Previously, the first override would take precedence, now it would be the second override from an env var. I'm not sure if I'd go as far as call the current precedence order a bug", this does seem like a bug to me. It's inconsistent behavior even if unlikely to be encountered. If yall don't think it's a bug then I'd agree with matthew that it's a breaking change, but sounds like a bug to me. Also sounds like it could be addressed separately however with the rest of tzemanovic's proposed changes.

I hope this helps move this forward in some direction. Just to summarize

  1. I'm in favor of merging
    1. (barring someone has evidence of a big performance regression, I know that was mentioned earlier)
  2. I'm also in favor of the direction tzemanovic's pr is taking and the (what think is a) bug it also addresses, but I think this can be addressed separately.
  3. Finally I'm in favor of lowering this default count dramatically but I think that's a breaking change (open to other opinions on that though).

I'll be on vacay for a couple weeks so that's all input I have for the time being

Copy link
Member

@matthew-russo matthew-russo left a comment

Choose a reason for hiding this comment

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

i'm fine merging as-is

@tzemanovic
Copy link
Contributor

unrelated change broke nightly build - fixing it in #389

@nipunn1313
Copy link
Contributor Author

I merged master here to reconcile the merge conflicts with master. Should be good to go!

@matthew-russo
Copy link
Member

Nightly build seems broken for an unrelated reason. will try to fix that today and then get this merged

@matthew-russo matthew-russo merged commit 5f87b3d into proptest-rs:master Jan 13, 2024
3 of 5 checks passed
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.

Default SizeRange too large
4 participants