Make quantity field human readable #891

matdexir · 2024-04-27T08:24:39Z

Description

Allows for human readable quantity field in the different json config, i.e, 10KB -> 10000.
This implementation still allows for the field being pure unsigned integers, so, it can be seen as offering an alternative configuration

Fixes #862

Type of change

Please delete options that aren't relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to
not work as expected)
This change requires a documentation update

How Has This Been Tested?

I tested it on a stripped version of basic_cas.json which allows for writing human readable units rather than large integer values.

Checklist

Updated documentation if needed
Tests added/amended
bazel test //... passes locally
PR is contained in a single commit, using git amend see some docs

This change is

allada

Thanks!

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-22.04, and 1 discussions need to be resolved

nativelink-config/src/serde_utils.rs line 54 at r1 (raw file):

        fn visit_str<E: de::Error>(self, mut v: &str) -> Result<Self::Value, E> {
            println!("We're supposed to be here {}", v);
            match v.chars().last().unwrap() {

I think we should make a new functions for these, like: convert_duration_with_shellexpand and convert_data_size_with_shellexpand

matdexir · 2024-04-28T05:10:50Z

Hi @allada,
Is there a specific test suite for this feature?
Can't seem to get bazel test ... to run properly.

allada

We don't have a test for this, but it'd be trivial to make one.

Pretty much it'd be to take this function (but change it so it passes in a string):

nativelink/src/bin/nativelink.rs

Line 808 in c1d0402

async fn get_config() -> Result<CasConfig, Box<dyn std::error::Error>> {

And put it into nativelink-config, then a config like this should properly populate:

{
  "global": {
    "default_digest_size_health_check": "5mb"
  }
}

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-22.04, and 1 discussions need to be resolved

matdexir · 2024-04-29T13:41:11Z

I am currently trying to get bazel test ... to run but for some reasons it fails. Here are the relevant logs:

ERROR: /home/matdexir/Files/Projects/nativelink/nativelink-config/BUILD.bazel:9:13: Rustfmt nativelink-config/nativelink-config.rustfmt.ok failed: (Exit 1): process_wrapper failed: error executing Rustfmt command (from target //nativelink-config:nativelink-config) bazel-out/k8-opt-exec-ST-13d3ddad9198/bin/external/rules_rust~0.42.1/util/process_wrapper/process_wrapper --touch-file bazel-out/k8-fastbuild/bin/nativelink-config/nativelink-config.rustfmt.ok -- ... (remaining 11 arguments skipped)
Diff in /tmp/nativelink/work/508fecf4260be788cf6e32983d75215d677579d6b11cb2bc2d23db421ee8cfb8/work/nativelink-config/src/serde_utils.rs at line 12:
 // See the License for the specific language governing permissions and
 // limitations under the License.

-use byte_unit::Byte;
-use humantime::parse_duration;
 use std::fmt;
 use std::marker::PhantomData;
 use std::str::FromStr;
Diff in /tmp/nativelink/work/508fecf4260be788cf6e32983d75215d677579d6b11cb2bc2d23db421ee8cfb8/work/nativelink-config/src/serde_utils.rs at line 20:

+use byte_unit::Byte;
+use humantime::parse_duration;
 use serde::{de, Deserialize, Deserializer};

I am not too sure what this means since it looks fine to my human eye. Maybe I forgot to update some settings?

allada

You need to run the formatter:
https://github.com/TraceMachina/nativelink/blob/main/CONTRIBUTING.md#fixing-rust-formatting

Looks great though! Maybe 1 test would be useful, but not required. I don't think we test the other fields here, so only do it if you want to learn how to write a new test.

Reviewed 2 of 3 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-22.04, and 4 discussions need to be resolved

-- commits line 2 at r2:
nit: Please expand this comment out a bit more to explain what's being added.

nativelink-config/src/serde_utils.rs line 172 at r2 (raw file):

        fn visit_str<E: de::Error>(self, v: &str) -> Result<Self::Value, E> {
            let byte_size = Byte::parse_str(v, true).map_err(de::Error::custom);

nit: put question mark here instead of the line below.

nativelink-config/src/serde_utils.rs line 174 at r2 (raw file):

            let byte_size = Byte::parse_str(v, true).map_err(de::Error::custom);
            let byte_size_str = byte_size?.as_u64().to_string();
            (*shellexpand::env(&byte_size_str).map_err(de::Error::custom)?)

This should be flipped around. Have it parse it from the shellexpand, then have it convert the Byte::parse_str. This will allow users to use environmental variables that contain things like 5mb and it'll expand properly.

nativelink-config/src/serde_utils.rs line 213 at r2 (raw file):

            let duration = parse_duration(v).map_err(de::Error::custom);
            let duration_str = duration?.as_secs().to_string();
            (*shellexpand::env(&duration_str).map_err(de::Error::custom)?)

nit: ditto from above.

allada

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 3 discussions need to be resolved

nativelink-config/src/serde_utils.rs line 174 at r3 (raw file):

            let expanded = (*shellexpand::env(v).map_err(de::Error::custom)?).to_string();
            let byte_size = Byte::parse_str(expanded, true).map_err(de::Error::custom)?;
            let byte_size_str = byte_size.as_u64().to_string();

Just convert this to a u128 then convert it directory into Self::Value.

nativelink-config/src/serde_utils.rs line 212 at r3 (raw file):

            let expanded = (*shellexpand::env(v).map_err(de::Error::custom)?).to_string();
            let duration = parse_duration(&expanded).map_err(de::Error::custom)?;
            let duration_str = duration.as_secs().to_string();

ditto, don't add .to_string()

matdexir · 2024-05-01T13:37:07Z

The most convenient solution I came up with was:

fn visit_str<E: de::Error>(self, v: &str) -> Result<Self::Value, E> {
    let expanded = (*shellexpand::env(v).map_err(de::Error::custom)?).to_string();
    let byte_size = Byte::parse_str(expanded, true).map_err(de::Error::custom)?;
    let byte_size_u128 = byte_size.as_u128();
    T::try_from(byte_size_u128.try_into().map_err(de::Error::custom)?)
       .map_err(de::Error::custom)
}

What do you think?

Also for the duration section, casting it seconds yields a u64 value rather than u128. I was wondering if this would be ideal (Although I could use as u128)? Or maybe I could cast it into {milli, micro, nano}(they all yield u128) seconds and convert it into seconds accordingly?

allada

Either is fine. It appears internally to that library it uses u128, which is why I suggested u128.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 3 discussions need to be resolved

allada

Reviewed 1 of 4 files at r4.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 3 discussions need to be resolved

nativelink-config/tests/deserialization_test.rs line 1 at r4 (raw file):

use nativelink_config::serde_utils::{

nit: Please add copyright header.

nativelink-config/tests/deserialization_test.rs line 6 at r4 (raw file):

#[cfg(test)]
mod tests {

nit: To simplify this test a bit, can I suggest instead just write dummy configs in the test with the annotations (like in the config rust files), then convert json5-string to the structs then just ensure the structs match a ground truth?

matdexir · 2024-05-06T13:58:50Z

I'm currently missing a test case for duration values. I haven't found any config files that could make use of duration units from here. Any idea on where I could get one?

allada

I would instead just make the struct definition in the test file itself and decorate a few fields to test it. I would not test the config directly, that will create complex dependency issues.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 3 discussions need to be resolved

adam-singer

@

Reviewed 3 of 4 files at r2, 2 of 4 files at r4, 2 of 3 files at r5, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 3 discussions need to be resolved

allada

Awesome! Thanks for the changes!

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04

allada

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 1 discussions need to be resolved

a discussion (no related file):
It looks like there's some strange errors when building with bazel. Let me know if you need help figuring them out.

matdexir · 2024-05-09T13:33:57Z

That's odd 🤔
It works all fine on my local machine. Could you give me a reproducible build with docker?

FYI. I am using nix OS for dev.
Here are my specs:
OS: Arch Linux
Kernel: 6.8.7-arch1-1

allada

Are you building with bazel or cargo? I suggest trying with bazel.

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 1 discussions need to be resolved

aaronmondal

@matdexir I believe the Bazel errors are because the Cargo lockfile wasn't regenerated. Bazel reads the lockfile at

nativelink/MODULE.bazel

Line 53 in 64ed20a

cargo_lockfile = "//:Cargo.lock",

to determine which dependencies to fetch.

To re-sync the Cargo.toml files with the lockfile, trycargo generate-lockfile.

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, and 1 discussions need to be resolved

CLAassistant · 2024-05-11T04:35:15Z

All committers have signed the CLA.

MarcusSorealheis · 2024-05-17T07:31:06Z

@matdexir do you need a hand further here?

Add support for human readable quantity field in the different json config, i.e, 10KB -> 10000. This implementation still allows for the field being pure unsigned integers, so, it can be seen as offering an alternative configuration.

matdexir · 2024-05-19T06:57:54Z

I think that it should be fixed now. I also have updated it to match the latest release. Let me know if the issue still lingers

NOTE: Currently nativelink-store tests are failing but I am not too sure why since it should technically be out of scope for my current PR

cc: @allada @aaronmondal @MarcusSorealheis

MarcusSorealheis · 2024-05-19T07:38:06Z

looks like you need to rebase @matdexir

matdexir · 2024-05-20T14:21:20Z

Already did
It should be up to date with 0a33c83

MarcusSorealheis

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Cargo Dev / macos-13, Remote / large-ubuntu-22.04, and 1 discussions need to be resolved

MarcusSorealheis

Dismissed @allada from a discussion.
Reviewable status: complete! 2 of 1 LGTMs obtained

The bazel errors have subsided after a few updates to deps.

Add support for human readable quantity field in the different json config, i.e, 10KB -> 10000. This implementation still allows for the field being pure unsigned integers, so, it can be seen as offering an alternative configuration. Co-authored-by: Marcus Eagan <[email protected]>

allada reviewed Apr 28, 2024

View reviewed changes

matdexir force-pushed the human-readable-quantity branch from bd18969 to 07f285e Compare April 28, 2024 05:09

allada reviewed Apr 28, 2024

View reviewed changes

allada reviewed Apr 29, 2024

View reviewed changes

matdexir force-pushed the human-readable-quantity branch 2 times, most recently from cd9d91c to 050f837 Compare April 30, 2024 14:41

matdexir marked this pull request as ready for review April 30, 2024 14:42

allada requested changes Apr 30, 2024

View reviewed changes

allada reviewed May 1, 2024

View reviewed changes

matdexir force-pushed the human-readable-quantity branch 2 times, most recently from 5ff4bef to a30503b Compare May 2, 2024 13:50

allada reviewed May 6, 2024

View reviewed changes

matdexir force-pushed the human-readable-quantity branch from a30503b to f33f0d2 Compare May 6, 2024 13:50

allada reviewed May 6, 2024

View reviewed changes

adam-singer reviewed May 6, 2024

View reviewed changes

matdexir force-pushed the human-readable-quantity branch 2 times, most recently from 7997f4e to a33e511 Compare May 7, 2024 13:24

allada approved these changes May 8, 2024

View reviewed changes

allada previously requested changes May 8, 2024

View reviewed changes

allada reviewed May 10, 2024

View reviewed changes

aaronmondal reviewed May 10, 2024

View reviewed changes

matdexir force-pushed the human-readable-quantity branch from a33e511 to 5812149 Compare May 11, 2024 04:35

matdexir force-pushed the human-readable-quantity branch from 5812149 to cab911e Compare May 19, 2024 06:09

Make quantity field human readable

0611fcc

Add support for human readable quantity field in the different json config, i.e, 10KB -> 10000. This implementation still allows for the field being pure unsigned integers, so, it can be seen as offering an alternative configuration.

matdexir force-pushed the human-readable-quantity branch from cab911e to 0611fcc Compare May 19, 2024 06:40

vercel bot temporarily deployed to Preview June 3, 2024 03:13 Inactive

Merge branch 'main' into human-readable-quantity

5d6afb6

MarcusSorealheis force-pushed the human-readable-quantity branch from 7b87729 to 5d6afb6 Compare June 3, 2024 03:16

vercel bot temporarily deployed to Preview June 3, 2024 03:17 Inactive

MarcusSorealheis approved these changes Jun 3, 2024

View reviewed changes

MarcusSorealheis enabled auto-merge (squash) June 3, 2024 04:40

MarcusSorealheis reviewed Jun 3, 2024

View reviewed changes

MarcusSorealheis merged commit da2c4a7 into TraceMachina:main Jun 3, 2024
28 checks passed

MarcusSorealheis mentioned this pull request Jun 4, 2024

bump serde to more current version #966

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make quantity field human readable #891

Make quantity field human readable #891

matdexir commented Apr 27, 2024 •

edited

Loading

allada left a comment

matdexir commented Apr 28, 2024 •

edited

Loading

allada left a comment

matdexir commented Apr 29, 2024

allada left a comment

allada left a comment

matdexir commented May 1, 2024

allada left a comment

allada left a comment

matdexir commented May 6, 2024

allada left a comment

adam-singer left a comment

allada left a comment

allada left a comment

matdexir commented May 9, 2024 •

edited

Loading

allada left a comment

aaronmondal left a comment

CLAassistant commented May 11, 2024 •

edited

Loading

MarcusSorealheis commented May 17, 2024

matdexir commented May 19, 2024 •

edited

Loading

MarcusSorealheis commented May 19, 2024

matdexir commented May 20, 2024

MarcusSorealheis left a comment

MarcusSorealheis left a comment

Make quantity field human readable #891

Make quantity field human readable #891

Conversation

matdexir commented Apr 27, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist

allada left a comment

Choose a reason for hiding this comment

matdexir commented Apr 28, 2024 • edited Loading

allada left a comment

Choose a reason for hiding this comment

matdexir commented Apr 29, 2024

allada left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

matdexir commented May 1, 2024

allada left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

matdexir commented May 6, 2024

allada left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

matdexir commented May 9, 2024 • edited Loading

allada left a comment

Choose a reason for hiding this comment

aaronmondal left a comment

Choose a reason for hiding this comment

CLAassistant commented May 11, 2024 • edited Loading

MarcusSorealheis commented May 17, 2024

matdexir commented May 19, 2024 • edited Loading

MarcusSorealheis commented May 19, 2024

matdexir commented May 20, 2024

MarcusSorealheis left a comment

Choose a reason for hiding this comment

MarcusSorealheis left a comment

Choose a reason for hiding this comment

matdexir commented Apr 27, 2024 •

edited

Loading

matdexir commented Apr 28, 2024 •

edited

Loading

matdexir commented May 9, 2024 •

edited

Loading

CLAassistant commented May 11, 2024 •

edited

Loading

matdexir commented May 19, 2024 •

edited

Loading