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

Set --legacy_important_outputs to false by default. #14353

Closed

Conversation

benjaminp
Copy link
Collaborator

This flag reduces the largest proto size, which helps avoid sharp edges with remote execution systems (e.g., #12050).

RELNOTES[INC]: --legacy_important_outputs now has a default of false.

@google-cla google-cla bot added the cla: yes label Nov 30, 2021
This flag reduces the largest BES proto size, which helps avoid sharp edges with remote execution systems (e.g., bazelbuild#12050).

RELNOTES[INC]: --legacy_important_outputs now has a default of false.
@benjaminp benjaminp force-pushed the legacy_important_outputs branch from 88e62a4 to 6a95c58 Compare November 30, 2021 17:24
@oquenchil oquenchil added the team-Local-Exec Issues and PRs for the Execution (Local) team label Dec 7, 2021
@meisterT meisterT added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc and removed team-Local-Exec Issues and PRs for the Execution (Local) team labels Dec 7, 2021
@meisterT meisterT assigned michaeledgar and unassigned meisterT Dec 7, 2021
@michaeledgar
Copy link
Contributor

This is a good change for a future Bazel release. I'll start importing this change.

@sgowroji sgowroji requested a review from haxorz May 4, 2022 06:36
@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label May 4, 2022
@haxorz
Copy link
Contributor

haxorz commented May 4, 2022

@michaeledgar What's the status of this? Can it be merged into Bazel? In internal issue 212691656 I see you already prepped for a change in the default value.

@bazel-io bazel-io closed this in 577cfbc May 20, 2022
@benjaminp benjaminp deleted the legacy_important_outputs branch May 20, 2022 14:53
@ashleyrichter
Copy link

ResultStore customers, including TensorFlow, still depend on this. We can't make this change until we have a migration plan in place for them.

bazel-io pushed a commit that referenced this pull request May 20, 2022
*** Reason for rollback ***

Breaking ResultStore customers.

RELNOTES[INC]: --legacy_important_outputs default reverted to true.

*** Original change description ***

Set --legacy_important_outputs to false by default.

This flag reduces the largest proto size, which helps avoid sharp edges with remote execution systems (e.g., #12050).

Closes #14353.

PiperOrigin-RevId: 450067034
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
@keith
Copy link
Member

keith commented Feb 6, 2024

shouldn't result store users just flip this flag to true then?

@ashleyrichter
Copy link

We're working on migrating ResultStore off important_outputs. That should be done mid-2023.

In the meantime, I've found more users of this functionality that need to be migrated off it.

@brentleyjones
Copy link
Contributor

Regardless, those users can set --legacy_important_outputs in their .bazelrc. Most users would benefit from the flag being flipped. We aren't asking for the flag to be flipped and removed, just flipped.

@brentleyjones
Copy link
Contributor

cc: @bazelbuild/triage

@iancha1992
Copy link
Member

cc: @meteorcloudy @Wyverald

@ashleyrichter
Copy link

I think my team could own the flipping of this flag. I'm not sure how this interacts with Bazel breaking changes policy, since it is a breaking change albeit one that is fixable by reenabling the flag. I will consult BEP owners.

@brentleyjones
Copy link
Contributor

It can go into master just fine, but we might not be able to cherry-pick it into 7.x depending on the breaking change policy.

@ashleyrichter
Copy link

I also got confirmation from wyv@ that this should go into 8.0, which might be EoY.

Maybe We'll be able to migrate users before then.

copybara-service bot pushed a commit that referenced this pull request Sep 10, 2024
…alse`.

Fixes #14353

RELNOTES[INC]: --legacy_important_outputs is flipped to false. See #14353 for details

PiperOrigin-RevId: 673122098
Change-Id: Ic9074406ebb7f9ac11be19afc8555aba0f40db60
fweikert pushed a commit that referenced this pull request Sep 20, 2024
Baseline: ad2ea07

Incompatible changes:

  - --legacy_important_outputs is flipped to false. See #14353 for
    details

Important changes:

  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - Overrides in the root MODULE.bazel file are now ignored with
    `--ignore_dev_dependency`. (Overrides in non-root modules are
    already ignored.)
  - Added support for using a remote cache that evicts blobs and
    doesn't have AC integrity check (e.g. HTTP cache).

This release contains contributions from many people at Google, as well as Benjamin Peterson, Fabian Meumertzheim, Jacob Van De Weert, John Millikin.
copybara-service bot pushed a commit that referenced this pull request Sep 20, 2024
Baseline: ad2ea07

Incompatible changes:

  - --legacy_important_outputs is flipped to false. See #14353 for
    details

Important changes:

  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - Overrides in the root MODULE.bazel file are now ignored with
    `--ignore_dev_dependency`. (Overrides in non-root modules are
    already ignored.)
  - Added support for using a remote cache that evicts blobs and
    doesn't have AC integrity check (e.g. HTTP cache).

This release contains contributions from many people at Google, as well as Benjamin Peterson, Fabian Meumertzheim, Jacob Van De Weert, John Millikin.
fmeum pushed a commit to fmeum/bazel that referenced this pull request Sep 20, 2024
Baseline: ad2ea07

Incompatible changes:

  - --legacy_important_outputs is flipped to false. See bazelbuild#14353 for
    details

Important changes:

  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - Overrides in the root MODULE.bazel file are now ignored with
    `--ignore_dev_dependency`. (Overrides in non-root modules are
    already ignored.)
  - Added support for using a remote cache that evicts blobs and
    doesn't have AC integrity check (e.g. HTTP cache).

This release contains contributions from many people at Google, as well as Benjamin Peterson, Fabian Meumertzheim, Jacob Van De Weert, John Millikin.
iancha1992 pushed a commit that referenced this pull request Sep 20, 2024
Baseline: ad2ea07

Incompatible changes:

  - --legacy_important_outputs is flipped to false. See #14353 for
    details

Important changes:

  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - Overrides in the root MODULE.bazel file are now ignored with
    `--ignore_dev_dependency`. (Overrides in non-root modules are
    already ignored.)
  - Added support for using a remote cache that evicts blobs and
    doesn't have AC integrity check (e.g. HTTP cache).

This release contains contributions from many people at Google, as well as Benjamin Peterson, Fabian Meumertzheim, Jacob Van De Weert, John Millikin.
keertk pushed a commit that referenced this pull request Oct 3, 2024
Baseline: f4d92d4

Cherry picks:

   + 99434b1:
     Automated rollback of commit
     4607ad4.

Incompatible changes:

  - `ctx.resolve_tools` is no longer available by default, in
    preparation for complete removal. See
    #22249 for migration
    instructions. Use `--noincompatible_disallow_ctx_resolve_tools`
    to temporarily make it available again.
  - The `aquery` command now reports all potential inputs of actions
    that support input discovery, including the input headers of C++
    compilation actions and those explicitly marked as unused through
    the `unused_inputs_list` argument to `ctx.actions.run`. Set
    `--noinclude_pruned_inputs` to omit pruned inputs from `aquery`
    output when running it after action execution.
    RELNOTES[INC]: This is not a release note, but a reminder to
    remove the note for `--include_scheduling_dependencies`, which
    was introduced in the 8.x tree but won't make it into the final
    release.
  - `--zip_undeclared_test_outputs` now defaults to false, causing
    undeclared test outputs (i.e., files written to
    `$TEST_UNDECLARED_OUTPUTS_DIR` by a test) to be produced as a
    directory instead of a zip file.
  - --legacy_important_outputs is flipped to false. See #14353 for
    details

New features:

  - Bazel can now parse .scl files, a dialect of Starlark without
    Bazel-specific symbols.
  - Dormant dependencies and materializer functions are now available
    with the --experimental_dormant_deps flag.

Important changes:

  - Deleted native Android mobile-install
  - Repository rules instantiated in the same module extensions can
    now refer to each other by their extension-specified names in
    label attributes.
  - A new experimental flag,
    `--experimental_build_event_output_group_mode`, allows users to
    change how a given output group's files are reported in BEP. The
    current behavior is `FILESET` which populates
    `OutputGroup.file_sets`. Users may now specify `INLINE` to
    instead report files directly in the
    `TargetComplete`/`AspectComplete` event under
    `OutputGroup.inline_files`. Users may also specify `BOTH` to
    populate `OutputGroup.file_sets` and `OutputGroup.inline_files`.
  - Bazel no longer has the android_binary, android_library,
    android_device_script_fixture and android_host_service_fixture
    rules. Use https://github.com/bazelbuild/rules_android instead.
    See #23199
  - Bazel no longer has the android_sdk_repository rule. Use
    https://github.com/bazelbuild/rules_android instead.
  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - `--incompatible_remote_downloader_send_all_headers` is removed.
  - `--build_event_upload_max_threads` is removed.
  - `incompatible_remote_output_paths_relative_to_input_root` is
    removed.
  - The default value of
    `--experimental_remote_cache_compression_threshold` is changed to
    `100`.
  - Build without the Bytes is disabled when using HTTP cache.
  - Build without the Bytes is disabled when using HTTP cache.
  - Symlink trees are now created through direct filesystem calls by
    default, instead of delegated to a helper process. On Windows,
    this entails respecting the `--windows_enable_symlinks` flag,
    falling back to a copy when the flag is unset (the helper process
    always attempts to create symlinks, irrespective of the flag).
    Set `--noexperimental_inprocess_symlink_creation` to temporarily
    revert to the previous behavior, which will be removed in a
    future release.
  - By default, coverage artifacts will be reported inline in the
    `TargetComplete` event. To disable this behavior, pass
    `--experimental_build_event_output_group_mode=baseline.lcov=named_
    set_of_files_only`.
  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - Overrides in the root MODULE.bazel file are now ignored with
    `--ignore_dev_dependency`. (Overrides in non-root modules are
    already ignored.)
  - Added support for using a remote cache that evicts blobs and
    doesn't have AC integrity check (e.g. HTTP cache).
  - Undeclared test outputs are now reported individually in the BEP,
    unless zipping is enabled via `--zip_undeclared_test_outputs`.
  - The native version of android_tools_defaults_jar is no longer in
    Bazel. Use https://github.com/bazelbuild/rules_android instead.
  - Bazel fetch and vendor command now supports --target_pattern_file
    for specifying target patterns.
  - The compact execution log now stores runfiles in a more compact
    representation that should reduce the memory overhead and log
    output size, in particular for test spawns. This change required
    breaking changes to the (experimental) log format.
  - `override_repo` and `inject_repo` can be used to override and
    inject repos in module extensions.
  - Patches to the module file in `single_version_override` are now
    effective as long as the patch file lies in the root module.
  - If `--allowed_cpu_values` is set, the `--cpu` flag value is
    validated against it.

This release contains contributions from many people at Google, as well as Adam Azarchs, Alessandro Patti, Benjamin Peterson, Cornelius Riemenschneider, dependabot[bot], Fabian Meumertzheim, Fil-Den, George Gensure, hvd, Jacob Van De Weert, James Sharpe, Javier Maestro, Jay Conrod, John Millikin, Jordan Mele, Jordan Mele, Keith Smiley, Lior Gorelik, Luis Padron, Michael Siegrist, Nils Wireklint, PikachuHy, Sangita.Nalkar, Son Luong Ngoc, Thi Doan, Xdng Yng, xinyu.wang.
copybara-service bot pushed a commit that referenced this pull request Oct 3, 2024
Baseline: f4d92d4

Cherry picks:

   + 99434b1:
     Automated rollback of commit
     4607ad4.

Incompatible changes:

  - `ctx.resolve_tools` is no longer available by default, in
    preparation for complete removal. See
    #22249 for migration
    instructions. Use `--noincompatible_disallow_ctx_resolve_tools`
    to temporarily make it available again.
  - The `aquery` command now reports all potential inputs of actions
    that support input discovery, including the input headers of C++
    compilation actions and those explicitly marked as unused through
    the `unused_inputs_list` argument to `ctx.actions.run`. Set
    `--noinclude_pruned_inputs` to omit pruned inputs from `aquery`
    output when running it after action execution.
    RELNOTES[INC]: This is not a release note, but a reminder to
    remove the note for `--include_scheduling_dependencies`, which
    was introduced in the 8.x tree but won't make it into the final
    release.
  - `--zip_undeclared_test_outputs` now defaults to false, causing
    undeclared test outputs (i.e., files written to
    `$TEST_UNDECLARED_OUTPUTS_DIR` by a test) to be produced as a
    directory instead of a zip file.
  - --legacy_important_outputs is flipped to false. See #14353 for
    details

New features:

  - Bazel can now parse .scl files, a dialect of Starlark without
    Bazel-specific symbols.
  - Dormant dependencies and materializer functions are now available
    with the --experimental_dormant_deps flag.

Important changes:

  - Deleted native Android mobile-install
  - Repository rules instantiated in the same module extensions can
    now refer to each other by their extension-specified names in
    label attributes.
  - A new experimental flag,
    `--experimental_build_event_output_group_mode`, allows users to
    change how a given output group's files are reported in BEP. The
    current behavior is `FILESET` which populates
    `OutputGroup.file_sets`. Users may now specify `INLINE` to
    instead report files directly in the
    `TargetComplete`/`AspectComplete` event under
    `OutputGroup.inline_files`. Users may also specify `BOTH` to
    populate `OutputGroup.file_sets` and `OutputGroup.inline_files`.
  - Bazel no longer has the android_binary, android_library,
    android_device_script_fixture and android_host_service_fixture
    rules. Use https://github.com/bazelbuild/rules_android instead.
    See #23199
  - Bazel no longer has the android_sdk_repository rule. Use
    https://github.com/bazelbuild/rules_android instead.
  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - `--incompatible_remote_downloader_send_all_headers` is removed.
  - `--build_event_upload_max_threads` is removed.
  - `incompatible_remote_output_paths_relative_to_input_root` is
    removed.
  - The default value of
    `--experimental_remote_cache_compression_threshold` is changed to
    `100`.
  - Build without the Bytes is disabled when using HTTP cache.
  - Build without the Bytes is disabled when using HTTP cache.
  - Symlink trees are now created through direct filesystem calls by
    default, instead of delegated to a helper process. On Windows,
    this entails respecting the `--windows_enable_symlinks` flag,
    falling back to a copy when the flag is unset (the helper process
    always attempts to create symlinks, irrespective of the flag).
    Set `--noexperimental_inprocess_symlink_creation` to temporarily
    revert to the previous behavior, which will be removed in a
    future release.
  - By default, coverage artifacts will be reported inline in the
    `TargetComplete` event. To disable this behavior, pass
    `--experimental_build_event_output_group_mode=baseline.lcov=named_
    set_of_files_only`.
  - Uploading local action results to a disk or remote cache now
    occurs in the background whenever possible, potentially
    unblocking the execution of followup actions. Set
    `--noremote_cache_async` to revert to the previous behavior.
  - Overrides in the root MODULE.bazel file are now ignored with
    `--ignore_dev_dependency`. (Overrides in non-root modules are
    already ignored.)
  - Added support for using a remote cache that evicts blobs and
    doesn't have AC integrity check (e.g. HTTP cache).
  - Undeclared test outputs are now reported individually in the BEP,
    unless zipping is enabled via `--zip_undeclared_test_outputs`.
  - The native version of android_tools_defaults_jar is no longer in
    Bazel. Use https://github.com/bazelbuild/rules_android instead.
  - Bazel fetch and vendor command now supports --target_pattern_file
    for specifying target patterns.
  - The compact execution log now stores runfiles in a more compact
    representation that should reduce the memory overhead and log
    output size, in particular for test spawns. This change required
    breaking changes to the (experimental) log format.
  - `override_repo` and `inject_repo` can be used to override and
    inject repos in module extensions.
  - Patches to the module file in `single_version_override` are now
    effective as long as the patch file lies in the root module.
  - If `--allowed_cpu_values` is set, the `--cpu` flag value is
    validated against it.

This release contains contributions from many people at Google, as well as Adam Azarchs, Alessandro Patti, Benjamin Peterson, Cornelius Riemenschneider, dependabot[bot], Fabian Meumertzheim, Fil-Den, George Gensure, hvd, Jacob Van De Weert, James Sharpe, Javier Maestro, Jay Conrod, John Millikin, Jordan Mele, Jordan Mele, Keith Smiley, Lior Gorelik, Luis Padron, Michael Siegrist, Nils Wireklint, PikachuHy, Sangita.Nalkar, Son Luong Ngoc, Thi Doan, Xdng Yng, xinyu.wang.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Core Skyframe, bazel query, BEP, options parsing, bazelrc
Projects
None yet
Development

Successfully merging this pull request may close these issues.