Skip to content

Deny internal execution order ambiguities in CI #7386

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.

Sign up for GitHub

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

Jump to bottom
Open
2 of 4 tasks
alice-i-cecile opened this issue Jan 27, 2023 · 1 comment
Open
2 of 4 tasks

Deny internal execution order ambiguities in CI #7386

alice-i-cecile opened this issue Jan 27, 2023 · 1 comment
Labels
A-Build-System Related to build systems or continuous integration A-ECS Entities, components, systems, and events C-Testing A change that impacts how we test Bevy or how users test their apps D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 27, 2023 •
edited
Loading

What problem does this solve or what need does it fill?

As discussed in #7383, internal execution order ambiguities are genuinely bad.

They often represent subtle bugs, break determinism for end users, and result in noisy logs for users.

What solution would you like?

  1. Recreate the enum from Improvements to execution order ambiguity reporting #4299 to create configurable levels of system execution order ambiguity reporting / strictness first.
  2. Run the example from [Merged by Bors] - Reduce internal system order ambiguities, and add an example explaining them #7383 in CI.
  3. Ensure that this example panics if any ambiguities are found

To actually get this to pass, we need more powerful tools to resolve or ignore ambiguities between plugins that aren't aware of them.
Once #7267 is merged, we should have those.

Then, add some form of IntegrationPlugin to DefaultPlugins that uses system set configuration + feature flags to resolve or ignore cross-plugin ambiguities.

What alternative(s) have you considered?

Do this manually before every release 🥲

We could just ignore ambiguities on the offending systems completely, but that's definitely incorrect for important systems like UI layout and animation.

Current status

  • clean up tests in #146760
  • resolve all main app ambiguities and set the allowed number to 0
  • figure out how to count the number of ambiguities in the RenderApp and make sure they don't increase
  • resolve all render app ambiguities and set the allowed number to 0
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Build-System Related to build systems or continuous integration S-Blocked This cannot move forward until something else changes C-Testing A change that impacts how we test Bevy or how users test their apps labels Jan 27, 2023
@alice-i-cecile alice-i-cecile mentioned this issue Jan 29, 2023
Closed
1 task
@BD103 BD103 mentioned this issue Mar 21, 2024
Closed
This was referenced Jun 20, 2024
Merged
Closed
@Vrixyz
Copy link
Member

Vrixyz commented Jul 8, 2024

#13950 adds a dedicated example for ambiguity detection, which is run on CI.

You'll notice on the PR a list of disabled schedule labels, which you can clear to get a full list of current ambiguities.

github-merge-queue bot pushed a commit that referenced this issue Jul 17, 2024
@Vrixyz @janhohenheim
Test for ambiguous system ordering in CI (#13950)
26fc4c7 
Progress towards #7386.

Following discussion
https://discord.com/channels/691052431525675048/1253260494538539048/1253387942311886960

This Pull Request adds an example to detect system order ambiguities,
and also asserts none exist.

A lot of schedules are ignored in ordered to have the test passing, we
should thrive to make them pass, but in other pull requests.

<details><summary>example output <b>summary</b>, without ignored
schedules</summary>
<p>

```txt
$ cargo run --example ambiguity_detection 2>&1 | grep -C 1 "pairs of syst"
2024-06-21T13:17:55.776585Z  WARN bevy_ecs::schedule::schedule: Schedule First has ambiguities.
1 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- bevy_time::time_system (in set TimeSystem) and bevy_ecs::event::event_update_system (in set EventUpdates)
--
2024-06-21T13:17:55.782265Z  WARN bevy_ecs::schedule::schedule: Schedule PreUpdate has ambiguities.
11 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- bevy_pbr::prepass::update_mesh_previous_global_transforms and bevy_asset::server::handle_internal_asset_events
--
2024-06-21T13:17:55.809516Z  WARN bevy_ecs::schedule::schedule: Schedule PostUpdate has ambiguities.
63 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- bevy_ui::accessibility::image_changed and bevy_ecs::schedule::executor::apply_deferred
--
2024-06-21T13:17:55.816287Z  WARN bevy_ecs::schedule::schedule: Schedule Last has ambiguities.
3 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- bevy_gizmos::update_gizmo_meshes<bevy_gizmos::aabb::AabbGizmoConfigGroup> (in set UpdateGizmoMeshes) and bevy_gizmos::update_gizmo_meshes<bevy_gizmos::light::LightGizmoConfigGroup> (in set UpdateGizmoMeshes)
--
2024-06-21T13:17:55.831074Z  WARN bevy_ecs::schedule::schedule: Schedule ExtractSchedule has ambiguities.
296 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- bevy_render::extract_component::extract_components<bevy_sprite::SpriteSource> and bevy_render::render_asset::extract_render_asset<bevy_sprite::mesh2d::material::PreparedMaterial2d<bevy_sprite::mesh2d::color_material::ColorMaterial>>
```

</p>
</details> 

To try locally: 
```sh
CI_TESTING_CONFIG="./.github/example-run/ambiguity_detection.ron" cargo run --example ambiguity_detection --features "bevy_ci_testing,trace,trace_chrome"
```

---------

Co-authored-by: Jan Hohenheim <[email protected]>
@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! and removed S-Blocked This cannot move forward until something else changes labels Aug 15, 2024
@alice-i-cecile alice-i-cecile changed the title Deny internal exectuion order ambiguities in CI Deny internal execution order ambiguities in CI Aug 15, 2024
This was referenced Aug 15, 2024
Merged
Merged
Merged
github-merge-queue bot pushed a commit that referenced this issue Aug 15, 2024
@alice-i-cecile
Don't ask for ResMut in queue_view_auto_exposure_pipelines (#14762)
dbf0d70 
This was creating a spurious ambiguity: `PipelineCache` uses interior
mutability.

Spotted as part of #7386.
github-merge-queue bot pushed a commit that referenced this issue Aug 15, 2024
@alice-i-cecile @janhohenheim
Improve ambiguity detection example / test (#14760)
0c126ee 
# Objective

While tackling #7386, I noticed
a few nits / frustrations with the existing testing infrastructure.

Rather than mixing those changes in with much more challenging to review
changes to reduce ambiguities, I've split this work into its own PR.

## Solution

Substantially simplify the `ambiguity_detection` test code, and reduce
the verbosity of logging.

## Testing

When run locally, the test functions as expected, with somewhat cleaner
logging.

---------

Co-authored-by: Jan Hohenheim <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Aug 15, 2024
@alice-i-cecile
Add RenderSet::FinalCleanup for World::clear_entities (#14764)
a2fc9de 
# Objective

`World::clear_entities` is ambiguous with all of the other systems in
`RenderSet::Cleanup` because it access `&mut World`.

## Solution

I've added another system set variant, and made sure that this runs
after everything else.
 
## Testing

The `ambiguity_detection` example

## Migration Guide

`World::clear_entities` is now part of `RenderSet::PostCleanup` rather
than `RenderSet::Cleanup`. Your cleanup systems should likely stay in
`RenderSet::Cleanup`.

## Additional context

Spotted when working on #7386: this was responsible for a large number
of ambiguities.

This should be removed if / when #14449 is merged: there's no need to
call `clear_entities` at all if the rendering world is retained!
This was referenced Aug 15, 2024
Merged
Closed
github-merge-queue bot pushed a commit that referenced this issue Aug 15, 2024
@alice-i-cecile
Remove manual apply_deferred in bevy_ui (#14768)
5243fe6 
# Objective

This `apply_deferred` doesn't seem to have any effect, pointlessly
restricts parallelism and is responsible for a large number of system
order ambiguities. Spotted as part of #7386.

## Solution

Remove it.

This is the *only* manual apply_deferred in the code base currently.

## Testing

I've checked various UI examples and `split_screen`, and couldn't
discern any difference.

This looks like a remnant of a `(a, apply_deferred, b).chain()` pattern
where `b` got removed, leaving us with a weird vestige.
@alice-i-cecile alice-i-cecile mentioned this issue Sep 3, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Sep 3, 2024
@alice-i-cecile
Remove all existing system order ambiguities in DefaultPlugins (#15031
4ac2a63 
)

# Objective

As discussed in #7386, system
order ambiguities within `DefaultPlugins` are a source of bugs in the
engine and badly pollute diagnostic output for users.

We should eliminate them!

This PR is an alternative to #15027: with all external ambiguities
silenced, this should be much less prone to merge conflicts and the test
output should be much easier for authors to understand.

Note that system order ambiguities are still permitted in the
`RenderApp`: these need a bit of thought in terms of how to test them,
and will be fairly involved to fix. While these aren't *good*, they'll
generally only cause graphical bugs, not logic ones.

## Solution

All remaining system order ambiguities have been resolved.
Review this PR commit-by-commit to see how each of these problems were
fixed.

## Testing

`cargo run --example ambiguity_detection` passes with no panics or
logging!
@alice-i-cecile alice-i-cecile mentioned this issue Oct 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Build-System Related to build systems or continuous integration A-ECS Entities, components, systems, and events C-Testing A change that impacts how we test Bevy or how users test their apps D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Vrixyz @alice-i-cecile