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

Convert Machine::speedFactor from a non-neg int to a non-neg float #9841

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

Ericson2314
Copy link
Member

Motivation

The short motivation is to match Hydra, so we can de-dup.

Context

The long version is layed out in
#9840.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

The short motivation is to match Hydra, so we can de-dup.

The long version is layed out in
NixOS#9840.
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Yay, less spurious duplication!

src/libstore/machines.cc Show resolved Hide resolved
@thufschmitt thufschmitt merged commit 69d0ae2 into NixOS:master Jan 24, 2024
8 checks passed
@Ericson2314 Ericson2314 deleted the float-speed-factor branch January 24, 2024 15:04
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Jan 27, 2024

@Ericson2314 @thufschmitt this confuses me a lot. I was just about to update #9526, which is affected by this change (I let it pass that this PR did not update reference documentation because it barely exists yet). I was about to fix it up, but now I'm at loss what to write. Because, which strings are acceptable for being parsed as a float? I can't find any reference documentation for that. My creeping suspicion is that no one actually knows. And seriously, who's supposed to understand what the implementation of lexical_cast actually does?

Why do we have to make the speed factor a float in Nix? Why can't we make it an int in Hydra? uint is easy to understand, sequence of arabic numerals. float makes everyone's life miserable (except maybe people who're really into numerics). I think this is a regression in user experience and I'm considering to open a revert PR.

@Ericson2314
Copy link
Member Author

I am fine if it is just documented as an integer. I made it a float so that I wouldn't be making a breaking change to anyone's Hydra setup. The Hydra parser is in fact not yet replaced by the Nix one; just the data type is.

@fricklerhandwerk
Copy link
Contributor

That sounds like adding another bit to the iceberg. It's such a seemingly small thing, but where do we stop if we let this one through? We're constantly discussing about keeping the Nix API surface narrow, and suddenly this. We don't have by far as strict ideas about stability for Hydra. Until you started pouring in PRs it was on life support anyway, we may as well break that one piece.

@Ericson2314
Copy link
Member Author

For remote building, I generally consider Hydra more battled-tested than Nix. The speed factor is just used as a scheduling heuristic: thing like the current machine load are already quite non-deterministic so I don't think extreme precision in that useful here, and a float is useful so you can say some remote build is just a wee bit faster than another. Certainly to me it's nothing like that oddball behavior in the language itself which is a much more serious problem.

@thufschmitt
Copy link
Member

This is a very minor and advanced use-case. Consistency is the main desirable property, and it's easier to make Nix a bit more lenient than Hydra stricter. I don't think there's a need to think about it any more than that.

tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
Convert `Machine::speedFactor` from a non-neg int to a non-neg float

(cherry picked from commit 69d0ae2)
Change-Id: I2afb5cf9e4fe1384985c58353946135c3d102b42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants