Allow nativelink flake module to upload results #1369
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronmondal
temporarily deployed
to
production
GitHub Actions
Inactive
aaronmondal
temporarily deployed
to
production
GitHub Actions
Inactive
aaronmondal
had a problem deploying
to
production
GitHub Actions
Error
aaronmondal
commented
Sep 27, 2024
There was a problem hiding this comment.
+@SchahinRohani tested here: https://github.com/TraceMachina/nativelink/actions/runs/11061595123/job/30736559106
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 2 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache (LRE), Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, Web Platform Deployment / macos-14, Web Platform Deployment / ubuntu-24.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @SchahinRohani)
After verifying that the LRE workflows behave as intended it's safe to enable this by default.
aaronmondal
force-pushed
the
allow-uploading-results
branch
from
40226c3
to
c5f41f4
Compare
aaronmondal
temporarily deployed
to
production
GitHub Actions
Inactive
aaronmondal
temporarily deployed
to
production
GitHub Actions
Inactive
aaronmondal
temporarily deployed
to
production
GitHub Actions
Inactive
SchahinRohani
approved these changes
Sep 27, 2024
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache (LRE), Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, Web Platform Deployment / macos-14, Web Platform Deployment / ubuntu-24.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 27, 2024
…#1369)" This reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main.
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 27, 2024
…#1369)" This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main.
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 27, 2024
…#1369)" This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes TraceMachina#1371
aaronmondal
added a commit
that referenced
this pull request
Sep 27, 2024
This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes #1371
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 29, 2024
* Revert "Allow nativelink flake module to upload results (TraceMachina#1369)" (TraceMachina#1372) This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes TraceMachina#1371 * Introduce reproducible branch-based coverage Reports may now be build via: ```nix nix build .#nativelinkCoverageForHost ``` The `result` symlink then contains the contents of a webpage to view the existing reports. Pushes to main publish the site at tracemachina.github.io/nativelink. Reports are built in release mode to closely resemble production coverage of the testsuite. This also means that most worker tests are ignored via a new `nix` feature to make the testsuite run in nix sandboxes. It's not ideal, but accurately reflects our production coverage guarantees. A future resolution for this might be to implement more elaborate mocking functionality for `nativelink-worker`. Coverage leverages nix caching, but not Bazel caching. While Bazel has builtin support for coverage, the reports were not satisfactory as they rely on outdated gcov toolchains with vague hermeticity guarantees and unsatisfactory implementations for llvm-cov-based workflows (e.g. no branch-based coverage for rust and no story around coverage for heterogeneous code). Mid-term we should implement "fast development" coverage via Bazel alongside the "slow production" coverage via Nix. As next steps we should continuously publish the generated HTML pages via the web infrastructure and add hooks to report regressions.
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 30, 2024
* Revert "Allow nativelink flake module to upload results (TraceMachina#1369)" (TraceMachina#1372) This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes TraceMachina#1371 * Introduce reproducible branch-based coverage Reports may now be build via: ```nix nix build .#nativelinkCoverageForHost ``` The `result` symlink then contains the contents of a webpage to view the existing reports. Pushes to main publish the site at tracemachina.github.io/nativelink. Reports are built in release mode to closely resemble production coverage of the testsuite. This also means that most worker tests are ignored via a new `nix` feature to make the testsuite run in nix sandboxes. It's not ideal, but accurately reflects our production coverage guarantees. A future resolution for this might be to implement more elaborate mocking functionality for `nativelink-worker`. Coverage leverages nix caching, but not Bazel caching. While Bazel has builtin support for coverage, the reports were not satisfactory as they rely on outdated gcov toolchains with vague hermeticity guarantees and unsatisfactory implementations for llvm-cov-based workflows (e.g. no branch-based coverage for rust and no story around coverage for heterogeneous code). Mid-term we should implement "fast development" coverage via Bazel alongside the "slow production" coverage via Nix. As next steps we should continuously publish the generated HTML pages via the web infrastructure and add hooks to report regressions.
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 30, 2024
* Revert "Allow nativelink flake module to upload results (TraceMachina#1369)" (TraceMachina#1372) This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes TraceMachina#1371 * Introduce reproducible branch-based coverage Reports may now be build via: ```nix nix build .#nativelinkCoverageForHost ``` The `result` symlink then contains the contents of a webpage to view the existing reports. Pushes to main publish the site at tracemachina.github.io/nativelink. Reports are built in release mode to closely resemble production coverage of the testsuite. This also means that most worker tests are ignored via a new `nix` feature to make the testsuite run in nix sandboxes. It's not ideal, but accurately reflects our production coverage guarantees. A future resolution for this might be to implement more elaborate mocking functionality for `nativelink-worker`. Coverage leverages nix caching, but not Bazel caching. While Bazel has builtin support for coverage, the reports were not satisfactory as they rely on outdated gcov toolchains with vague hermeticity guarantees and unsatisfactory implementations for llvm-cov-based workflows (e.g. no branch-based coverage for rust and no story around coverage for heterogeneous code). Mid-term we should implement "fast development" coverage via Bazel alongside the "slow production" coverage via Nix. As next steps we should continuously publish the generated HTML pages via the web infrastructure and add hooks to report regressions.
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 30, 2024
* Revert "Allow nativelink flake module to upload results (TraceMachina#1369)" (TraceMachina#1372) This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes TraceMachina#1371 * Introduce reproducible branch-based coverage Reports may now be build via: ```nix nix build .#nativelinkCoverageForHost ``` The `result` symlink then contains the contents of a webpage to view the existing reports. Pushes to main publish the site at tracemachina.github.io/nativelink. Reports are built in release mode to closely resemble production coverage of the testsuite. This also means that most worker tests are ignored via a new `nix` feature to make the testsuite run in nix sandboxes. It's not ideal, but accurately reflects our production coverage guarantees. A future resolution for this might be to implement more elaborate mocking functionality for `nativelink-worker`. Coverage leverages nix caching, but not Bazel caching. While Bazel has builtin support for coverage, the reports were not satisfactory as they rely on outdated gcov toolchains with vague hermeticity guarantees and unsatisfactory implementations for llvm-cov-based workflows (e.g. no branch-based coverage for rust and no story around coverage for heterogeneous code). Mid-term we should implement "fast development" coverage via Bazel alongside the "slow production" coverage via Nix. As next steps we should continuously publish the generated HTML pages via the web infrastructure and add hooks to report regressions.
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 30, 2024
* Revert "Allow nativelink flake module to upload results (TraceMachina#1369)" (TraceMachina#1372) This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes TraceMachina#1371 * Introduce reproducible branch-based coverage Reports may now be build via: ```nix nix build .#nativelinkCoverageForHost ``` The `result` symlink then contains the contents of a webpage to view the existing reports. Pushes to main publish the site at tracemachina.github.io/nativelink. Reports are built in release mode to closely resemble production coverage of the testsuite. This also means that most worker tests are ignored via a new `nix` feature to make the testsuite run in nix sandboxes. It's not ideal, but accurately reflects our production coverage guarantees. A future resolution for this might be to implement more elaborate mocking functionality for `nativelink-worker`. Coverage leverages nix caching, but not Bazel caching. While Bazel has builtin support for coverage, the reports were not satisfactory as they rely on outdated gcov toolchains with vague hermeticity guarantees and unsatisfactory implementations for llvm-cov-based workflows (e.g. no branch-based coverage for rust and no story around coverage for heterogeneous code). Mid-term we should implement "fast development" coverage via Bazel alongside the "slow production" coverage via Nix. As next steps we should continuously publish the generated HTML pages via the web infrastructure and add hooks to report regressions.
aaronmondal
added a commit
to aaronmondal/nativelink
that referenced
this pull request
Sep 30, 2024
* Revert "Allow nativelink flake module to upload results (TraceMachina#1369)" (TraceMachina#1372) This partially reverts commit 9600839. The original idea was to implement a `readonly` setting to dynamically configure `--remote_upload_local_results`. While this is fairly straightforward to implement it turned out that the UX implications around the environment variables/scripts/files to configure this are nontrivial and we need to evaluate the different approaches in more depth first. For now, revert to readonly by default and also add a small modifier to the relevant workflow so that the write-access workflow retains the ability to write artifacts on pushes to main. Fixes TraceMachina#1371 * Introduce reproducible branch-based coverage Reports may now be build via: ```nix nix build .#nativelinkCoverageForHost ``` The `result` symlink then contains the contents of a webpage to view the existing reports. Pushes to main publish the site at tracemachina.github.io/nativelink. Reports are built in release mode to closely resemble production coverage of the testsuite. This also means that most worker tests are ignored via a new `nix` feature to make the testsuite run in nix sandboxes. It's not ideal, but accurately reflects our production coverage guarantees. A future resolution for this might be to implement more elaborate mocking functionality for `nativelink-worker`. Coverage leverages nix caching, but not Bazel caching. While Bazel has builtin support for coverage, the reports were not satisfactory as they rely on outdated gcov toolchains with vague hermeticity guarantees and unsatisfactory implementations for llvm-cov-based workflows (e.g. no branch-based coverage for rust and no story around coverage for heterogeneous code). Mid-term we should implement "fast development" coverage via Bazel alongside the "slow production" coverage via Nix. As next steps we should continuously publish the generated HTML pages via the web infrastructure and add hooks to report regressions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After verifying that the LRE workflows behave as intended it's safe to enable this by default.
This change is