deployment-examples: chromium #786

adam-singer · 2024-03-22T14:38:34Z

Description

Creating an example of how to build chromium unit tests using nativelink.

Fixes # (issue)

#779

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?

Please also list any relevant details for your test configuration

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

aaronmondal

Generally LGTM. I think it's fine if this isn't perfect initially. We should probably deduplicate all the unspecific K8s files. Maybe symlinks?

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, pre-commit-checks

-- commits line 3 at r1:
nit: Looks like this is some residue from rebasing

deployment-examples/chromium/03_build_chrome_tests.sh line 10 at r1 (raw file):

fi

if [ -d "${HOME}/chromium" ]; then

nit: I'm not sure how compatible this would be, but we might be able to put this into a Dockerfile which is based on Ubuntu, similar to https://github.com/TraceMachina/nativelink/pull/730/files#diff-934dfe8efcc8cf1ba1d5f7f24ff1c0568e34832676fa950de194344f2d2172a7

No strong opinion though.

deployment-examples/chromium/README.md line 1 at r1 (raw file):

# Chromium example

nit: Consider adding a > [!WARN] section to clarify that this only works on Ubuntu hosts. There is also some improved documentation at https://github.com/TraceMachina/nativelink/pull/730/files#diff-006f58c2e5cef6ef1bc72e75b151f7c88208e248e1a9a963816390b8fa530e14 that could be helpful as general reference.

deployment-examples/chromium/README.md line 43 at r1 (raw file):

This probably shouldn't be ignored by vale. If there are any words missing from the vocabulary (like "Chromium") you can add it to https://github.com/TraceMachina/nativelink/blob/main/.github/styles/config/vocabularies/TraceMachina/accept.txt. This way we can also make sure that we don't accidentally use the wrong capitalization (not sure whether it's "chromium" or "Chromium").

FYI words in backticks (e.g. ActionScheduler) are also ignored by vale, but this tends to be more useful in API docs.

deployment-examples/chromium/scheduler.json line 29 at r1 (raw file):

      // TODO(adams): max_bytes_per_stream
      "simple": {
        "supported_platform_properties": {

nit: Do we need all of this? If so, it might make sense to document the importance of this section.

deployment-examples/chromium/worker-chromium.json line 56 at r1 (raw file):

      "platform_properties": {
        "cpu_count": {
          "values": ["16"],

nit: Could nproc be a better choice?

nativelink/deployment-examples/docker-compose/worker.json

Line 54 in a6976e0

"cpu_count": {

deployment-examples/chromium/worker-chromium.yaml line 21 at r1 (raw file):

          env:
            - name: RUST_LOG
              value: debug,h2=off,tower=off,hyper=off

nit: Reminder to double check the log level to whatever we think is best for this example. No strong opinion on the concrete value.

deployment-examples/kubernetes/worker-lre-cc.json.template line 52 at r1 (raw file):

        "ac_store": "GRPC_LOCAL_AC_STORE",
      },
      "work_directory": "~/.cache/nativelink/work",

I believe this is a residue from rebasing.

deployment-examples/kubernetes/worker-lre-java.json.template line 28 at r1 (raw file):

          "filesystem": {
            "content_path": "/tmp/.cache/nativelink/data-worker-test/content_path-cas",
            "temp_path": "/tmp/.cache/nativelink/data-worker-test/tmp_path-cas",

ditto

adam-singer

I usually have a bit of an aversion to symlinks checked in to code repos, not opposed but personally would be ok with duplication if we plan on refactoring these files later.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

-- commits line 3 at r1:

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Looks like this is some residue from rebasing

ack. will do

Done.

deployment-examples/chromium/03_build_chrome_tests.sh line 10 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: I'm not sure how compatible this would be, but we might be able to put this into a Dockerfile which is based on Ubuntu, similar to https://github.com/TraceMachina/nativelink/pull/730/files#diff-934dfe8efcc8cf1ba1d5f7f24ff1c0568e34832676fa950de194344f2d2172a7

No strong opinion though.

Same, I don't have a strong opinion. I do wonder if there a penalty for using docker on the client side where lots of files read/write in docker.

deployment-examples/chromium/README.md line 1 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Consider adding a > [!WARN] section to clarify that this only works on Ubuntu hosts. There is also some improved documentation at https://github.com/TraceMachina/nativelink/pull/730/files#diff-006f58c2e5cef6ef1bc72e75b151f7c88208e248e1a9a963816390b8fa530e14 that could be helpful as general reference.

Done.

deployment-examples/chromium/README.md line 43 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This probably shouldn't be ignored by vale. If there are any words missing from the vocabulary (like "Chromium") you can add it to https://github.com/TraceMachina/nativelink/blob/main/.github/styles/config/vocabularies/TraceMachina/accept.txt. This way we can also make sure that we don't accidentally use the wrong capitalization (not sure whether it's "chromium" or "Chromium").

FYI words in backticks (e.g. ActionScheduler) are also ignored by vale, but this tends to be more useful in API docs.

TY, was hoping that you spotted that and provided the dictionary to add words to

deployment-examples/chromium/scheduler.json line 29 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Do we need all of this? If so, it might make sense to document the importance of this section.

would need to revisit this, iirc there was problems with getting property matching correct

deployment-examples/chromium/worker-chromium.yaml line 21 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Reminder to double check the log level to whatever we think is best for this example. No strong opinion on the concrete value.

Done.

deployment-examples/kubernetes/worker-lre-cc.json.template line 52 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I believe this is a residue from rebasing.

yep, kept it tracking your branch, so next push will fix the extra commit/metadata in this review.

Done.

deployment-examples/kubernetes/worker-lre-java.json.template line 28 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

ditto

Done.

aaronmondal

Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

.github/styles/config/vocabularies/TraceMachina/accept.txt line 27 at r2 (raw file):

parsable
rebase
[Cc]hromium

nit: Consider only allowing capitalized "Chromium".

deployment-examples/chromium/03_build_chrome_tests.sh line 10 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

Same, I don't have a strong opinion. I do wonder if there a penalty for using docker on the client side where lots of files read/write in docker.

Let's leave this as is for now. We can change it later.

deployment-examples/chromium/README.md line 10 at r2 (raw file):

> will check if the image is Ubuntu and fail otherwise.

nit: One newline

aaronmondal

fyi you can exclude the keys from the failing pre-commit checks here:

nativelink/tools/pre-commit-hooks.nix

Line 45 in 09b32c9

# Integration testfiles not intended for production.

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

adam-singer

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

.github/styles/config/vocabularies/TraceMachina/accept.txt line 27 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Consider only allowing capitalized "Chromium".

Done.

deployment-examples/chromium/03_build_chrome_tests.sh line 10 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Let's leave this as is for now. We can change it later.

sgtm, I do have a different patch that does use docker client, we could roll that into this.
Done.

deployment-examples/chromium/README.md line 10 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: One newline

Done.

allada

FWIW, I'd rather use symlinks in code bases, as it makes code search more usable and reduces chance of "only changing in once place".

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

adam-singer

Used symlinks

Done.

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, Vercel, 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-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 1 discussions need to be resolved

deployment-examples/chromium/worker-chromium.json line 56 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Could nproc be a better choice?

nativelink/deployment-examples/docker-compose/worker.json

Line 54 in a6976e0

"cpu_count": {

Done.

adam-singer

@aaronmondal @allada using a symlink won't work for all of the script files since there are differences between them, we could parameterize out everything, I think that would be best for a later and muddle this example a bit.

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / 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-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 1 discussions need to be resolved

adam-singer

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

deployment-examples/chromium/03_build_chrome_tests.sh line 10 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

sgtm, I do have a different patch that does use docker client, we could roll that into this.
Done.

Done.

aaronmondal

Reviewed 1 of 38 files at r3, 40 of 40 files at r4, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks, and 1 discussions need to be resolved

deployment-examples/chromium/README.md line 10 at r4 (raw file):

> will check if the image is Ubuntu and fail otherwise.

All commands should be run from nix to ensure all dependencies exists in the environment.

nit exsist

zbirenbaum

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks, and 2 discussions need to be resolved

deployment-examples/chromium/03_build_chrome_tests.sh line 10 at r4 (raw file):

fi

if [ -d "${HOME}/chromium" ]; then

I ran into a problem on this line where the depot tools portion below didn't properly update my path and fetch wasn't found. Logging back in and starting nix again fixed the path issue, but then it assumed that the chromium checkout existed when it didn't since an empty directory was there.

I would change the check for existing chromium to be

if [ -d "${HOME}/chromium/src" ]; then

Especially since that's where it cd's anyways

notion-workspace · 2024-04-01T18:12:35Z

deployment-examples: chromium

adam-singer

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (22.04), pre-commit-checks, windows-2022 / stable, and 1 discussions need to be resolved

deployment-examples/chromium/03_build_chrome_tests.sh line 10 at r4 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

I ran into a problem on this line where the depot tools portion above didn't properly update my path and fetch wasn't found. Logging back in and starting nix again fixed the path issue, but then it assumed that the chromium checkout existed when it didn't since an empty directory was there.

I would change the check for existing chromium to be

if [ -d "${HOME}/chromium/src" ]; then

Especially since that's where it cd's anyways

Done.

deployment-examples/chromium/README.md line 10 at r4 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit exsist

Done.

zbirenbaum

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: pre-commit-checks

adam-singer

Reviewed 2 of 37 files at r5, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: pre-commit-checks

adam-singer

Reviewed 1 of 37 files at r5.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: pre-commit-checks

adam-singer

Reviewed 2 of 7 files at r2, 3 of 40 files at r4, 33 of 37 files at r5.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: pre-commit-checks

adam-singer

Reviewed 1 of 37 files at r5.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: pre-commit-checks

zbirenbaum

Reviewed 11 of 20 files at r1, 4 of 7 files at r2, 5 of 40 files at r4, 37 of 37 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained

adam-singer requested a review from aaronmondal March 22, 2024 14:38

aaronmondal approved these changes Mar 22, 2024

View reviewed changes

adam-singer commented Mar 25, 2024

View reviewed changes

adam-singer force-pushed the adams/generalize-lre-approach-branched branch from 67d3fea to 3344475 Compare March 25, 2024 17:14

adam-singer marked this pull request as ready for review March 25, 2024 17:15

adam-singer force-pushed the adams/generalize-lre-approach-branched branch from 2f820a0 to 67d3fea Compare March 25, 2024 17:16

aaronmondal approved these changes Mar 25, 2024

View reviewed changes

adam-singer commented Mar 26, 2024

View reviewed changes

adam-singer force-pushed the adams/generalize-lre-approach-branched branch from 3344475 to 138a602 Compare March 26, 2024 18:07

allada force-pushed the main branch 2 times, most recently from a2a06c2 to 09defc6 Compare March 26, 2024 18:19

allada reviewed Mar 27, 2024

View reviewed changes

adam-singer force-pushed the adams/generalize-lre-approach-branched branch 2 times, most recently from bd737c0 to ef7017e Compare March 27, 2024 18:21

adam-singer commented Mar 27, 2024

View reviewed changes

adam-singer force-pushed the adams/generalize-lre-approach-branched branch from ef7017e to 13298e7 Compare March 27, 2024 19:29

adam-singer commented Mar 27, 2024

View reviewed changes

aaronmondal approved these changes Mar 28, 2024

View reviewed changes

zbirenbaum requested changes Mar 28, 2024

View reviewed changes

adam-singer mentioned this pull request Apr 1, 2024

Generalize k8s workers #812

Merged

adam-singer force-pushed the adams/generalize-lre-approach-branched branch 2 times, most recently from 20d94f4 to d7f9cec Compare April 2, 2024 04:33

adam-singer commented Apr 2, 2024

View reviewed changes

zbirenbaum reviewed Apr 2, 2024

View reviewed changes

adam-singer commented Apr 2, 2024

View reviewed changes

Example of chromium using deployment scripts

a6eb4da

adam-singer force-pushed the adams/generalize-lre-approach-branched branch from d7f9cec to a6eb4da Compare April 2, 2024 16:59

zbirenbaum approved these changes Apr 2, 2024

View reviewed changes

adam-singer merged commit 0aa7f65 into TraceMachina:main Apr 2, 2024
26 checks passed

adam-singer deleted the adams/generalize-lre-approach-branched branch April 2, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deployment-examples: chromium #786

deployment-examples: chromium #786

adam-singer commented Mar 22, 2024 •

edited by allada

Loading

aaronmondal left a comment

adam-singer left a comment

aaronmondal left a comment

aaronmondal left a comment

adam-singer left a comment

allada left a comment

adam-singer left a comment

adam-singer left a comment

adam-singer left a comment

aaronmondal left a comment

zbirenbaum left a comment •

edited

Loading

notion-workspace bot commented Apr 1, 2024

adam-singer left a comment

zbirenbaum left a comment

adam-singer left a comment

adam-singer left a comment

adam-singer left a comment

adam-singer left a comment

zbirenbaum left a comment

deployment-examples: chromium #786

deployment-examples: chromium #786

Conversation

adam-singer commented Mar 22, 2024 • edited by allada Loading

Description

Type of change

How Has This Been Tested?

Checklist

aaronmondal left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

aaronmondal left a comment

Choose a reason for hiding this comment

aaronmondal 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

adam-singer left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

aaronmondal left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment • edited Loading

Choose a reason for hiding this comment

notion-workspace bot commented Apr 1, 2024

adam-singer left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

adam-singer commented Mar 22, 2024 •

edited by allada

Loading

zbirenbaum left a comment •

edited

Loading