Test for ambiguous system ordering in CI #13950

Vrixyz · 2024-06-20T18:29:26Z

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.

example output summary, without ignored schedules

$ 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>>

To try locally:

CI_TESTING_CONFIG="./.github/example-run/ambiguity_detection.ron" cargo run --example ambiguity_detection --features "bevy_ci_testing,trace,trace_chrome"

examples/ambiguity_detection.rs

alice-i-cecile

Awesome! Some early feedback for you. In addition to my comments, this should probably be an integration test, in the root tests folder.

I've also linked the issue for this in your PR description for you :)

Vrixyz · 2024-06-21T16:13:02Z

tests/ambiguity_detection.rs

+        Box::new(PostUpdate),
+        Box::new(Last),
+        Box::new(ExtractSchedule),
+        //Box::new(Render),


This works as an example, but I'm not sure how to enable that for a test ? @alice-i-cecile

Vrixyz · 2024-06-21T16:14:56Z

tests/ambiguity_detection.rs

+/// FIXME: bevy should not have any ambiguities, but it takes time to clean these up,
+/// so we're juste ignoring those for now.


I think that's a satisfying first approach, I'm not up to fixing all 300+ ambiguities, and merging this allows us to be careful not to introduce much more.

I could fine-tune this to say the amount of ambiguities should not be more than before 🤔 (by hardcoding the value I currently see, and we revisit if we make it better (or hardcode the value to be exactly the value I see, so we're raising aware-ness on that front), let me know if that's interesting.

We should probably have a tracking issue in addition to the fixme.

I can add a link to #7386 and consolidate the issue with an exact list as of now ; or instructions on how to get that list

hymm · 2024-06-21T16:23:35Z

tests/ambiguity_detection.rs

+    }
+    let sub_app = app.main();
+    assert_no_conflicting_systems(sub_app);
+    // RenderApp is not checked here, because it is not within the App at this point.


We could disable the PipelinedRenderPlugin to avoid this.

thanks for the pointer!

I'm actually not sure it's too useful to disable, as I check for RenderExtractApp, it should be comparable. of course it would be best to try out every configuration but I'll consider a good first approach is no one is having strong feelings.

I'm not sure I understand. The RenderExtractApp and the RenderApp have different schedules.

From a quick read of 'PipelinedRenderPlugin' it seemed like RenderApp is removed in favor of RenderExtractApp, which would make testing both impossible ?

But I'm probably wrong, I'm not sure where the RenderApp is gone 😅.

At the same time, I'm worried removing PipelinedRenderPlugin would remove some systems so they wouldn't be tested for ambiguity, so I assume your solution is a trade-off, but I lack knowledge to judge which is best.

RenderApp is much more important to test than RenderExtractApp. RenderExtractApp just has one system in it. RenderApp is all the rendering systems.

examples/ecs/ambiguity_detection.rs

github-actions · 2024-06-22T20:48:30Z

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Vrixyz · 2024-06-22T21:15:18Z

I'd love a way to iterate through all subapps, but sub_apps s currently not exposed, I'm not sure if there's a reason for that.

I'd prefer to let that idea as a follow-up.

alice-i-cecile · 2024-07-14T19:31:22Z

examples/ecs/ambiguity_detection.rs

@@ -0,0 +1,78 @@
+//! An example to confirm that `bevy` doesn't have system order ambiguity with [`DefaultPlugins`]
+//! This is run in CI to ensure that this doesn't regress again.


Is this comment true?

Yes, it is run in CI through ci examples check thanks to this file ; I believe I went through this because I failed at checking RenderApp with a simpler test.

I'll try again, maybe there's a way to test for RenderApp.

seems to be working as an integration test / hidden example

alice-i-cecile

I really want this to exist, even in a disabled form. That said, I'd really like to avoid merging this as an example. This is fundamentally a test, and should be located with them.

Two questions:

What are the blockers to making this an integration test?
Can we simplify this to simply track the number of ambiguous systems, and then fail if it doesn't match?

Vrixyz · 2024-07-15T20:58:32Z

What are the blockers to making this an integration test?

I gave it a new attempt, and it seems to be working, I'm not sure exactly how to set this up, I added it in the tests/ folder, but it's listed as a hidden example, is that correct ?

Can we simplify this to simply track the number of ambiguous systems, and then fail if it doesn't match?

I'm not sure it's wise, as a PR could add a system ordering ambiguity, but remove another one. Niche example, but still.

I added the logic to count "expected ambiguities" though, so the test is more explicit and thorough. That might backfire a bit, forcing contributors to edit the number, but if that "educates" contributors to be mindful of system ordering, that's probably worth it 🤔.

Vrixyz · 2024-07-15T21:44:41Z

Cargo.toml

@@ -365,6 +365,7 @@ ron = "0.8.0"
 flate2 = "1.0"
 serde = { version = "1", features = ["derive"] }
 bytemuck = "1.7"
+bevy_render = { path = "crates/bevy_render", version = "0.15.0-dev", default-features = false }


This is an important change, needed to get as much schedules as possible

Vrixyz · 2024-07-15T21:45:46Z

tests/ecs/ambiguity_detection.rs

+    vec![
+        Box::new(First),
+        Box::new(PreUpdate),
+        Box::new(Update),


only 1 ambiguity on Update at the time of testing, probably a good "low-hanging fruit"

2024-07-15T21:35:59.885229Z WARN update:main app:schedule{name=Main}:schedule{name=Update}: bevy_ecs::schedule::schedule: Schedule Update 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_app::terminal_ctrl_c_handler::TerminalCtrlCHandlerPlugin::exit_on_flag and bevy_dev_tools::ci_testing::systems::send_events conflict on: bevy_ecs::world::World

alice-i-cecile · 2024-07-17T14:04:57Z

Cargo.toml

@@ -2980,6 +2981,14 @@ description = "Demonstrates customizing default window settings"
 category = "Window"
 wasm = true

+[[example]]


What happens when this is removed? We shouldn't be listing tests as examples TBH.

That surprised me too, but I kept the behaviour as closely as what's existing (so in my opinion this is out of scope for this Pull Request):

Compare this to next example in Cargo.toml: resizing, which is used as a test, to be noted it has a metadata hidden = true (I believe that's used for the website to know which are published).

If we remove that block, we cannot run the example 😅, mostly a blocker for ci runs.

I believe using examples for testing is a workaround to be guaranteed main thread execution, because if I'm trying to run my example as a test: I have the following error:

$ cargo test --test ambiguity_detection Compiling bevy v0.15.0-dev (C:\Users\thier\Documents\perso\bevy) Finished `test` profile [unoptimized + debuginfo] target(s) in 17.69s Running tests/ecs/ambiguity_detection.rs (C:\Users\thier\Documents\cargo_target_dir\debug\deps\ambiguity_detection-eeb84a8dea010d13.exe) running 1 test 2024-07-17T14:43:32.965951Z INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Windows 10 Home", kernel: "19045", cpu: "AMD Ryzen 5 2600X Six-Core Processor", core_count: "6", memory: "32.0 GiB" } test ambiguity_detection ... FAILED failures: ---- ambiguity_detection stdout ---- thread 'ambiguity_detection' panicked at C:\Users\thier\.cargo\registry\src\index.crates.io-6f17d22bba15001f\winit-0.30.2\src\platform_impl\windows\event_loop.rs:180:13: Initializing the event loop outside of the main thread is a significant cross-platform compatibility hazard. If you absolutely need to create an EventLoop on a different thread, you can use the `EventLoopBuilderExtWindows::any_thread` function. note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I've tried one last thing, it seems possible to run those as test if we bypass the harness (thanks stackoverflow):

diff --git a/Cargo.toml b/Cargo.toml index d6eefaa91..533198ce6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2981,12 +2981,13 @@ description = "Demonstrates customizing default window settings" category = "Window" wasm = true -[[example]] +[[test]] name = "ambiguity_detection" path = "tests/ecs/ambiguity_detection.rs" doc-scrape-examples = true +harness = false -[package.metadata.example.ambiguity_detection] +[package.metadata.test.ambiguity_detection] hidden = true [[example]]

I'm not sure about its implications on the rest of bevy machinery though (example scraping, website building, ci tests...), so I'd be happy to study that in a follow up PR rather than in this one :)

Vrixyz · 2024-07-17T14:52:00Z

to be noted, I hardcoded the systems ambiguity counts to be correct when running with CI_TESTING_CONFIG="./.github/example-run/ambiguity_detection.ron" cargo run --example ambiguity_detection --features "bevy_ci_testing,trace,trace_chrome", because that's what's run in ci.

alice-i-cecile

Alright, I'm fine with this as a next step forward then. We can figure out how to improve the situation WRT main-thread execution and example-ness in the future.

janhohenheim

The functionality looks right and it would be good to get this in ASAP, so I'm approving. I'm not happy with the code quality of assert_no_conflicting_systems, but I'll leave it to the discretion of the maintainers on whether that should be a follow-up or a blocker.

janhohenheim · 2024-07-17T16:27:29Z

tests/ecs/ambiguity_detection.rs

+    app.update();
+
+    /// Returns the number of conflicting systems.
+    fn assert_no_conflicting_systems(sub_app: &SubApp) -> usize {


Some notes on this function

Could you please move it out of the main function?

The name is a total misnomer. The no should probably be no_new, but the what the function actually does is twofold

Assert that there are no unknown ambiguities

Count the amount of known ambiguities

A clean solution would probably be to return a struct containing the amount of known and unknown ambiguities. Then, the calling code can run the asserts on these two values. The function could then be called count_ambiguities

I did my best to provide a cleaner solution, there's .0 around but the result is better than before, with more exhaustive report rather than failing at the first unexpected ambiguity detected.

new report example (for a fail test):

1 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_app::terminal_ctrl_c_handler::TerminalCtrlCHandlerPlugin::exit_on_flag and bevy_dev_tools::ci_testing::systems::send_events conflict on: bevy_ecs::world::World thread 'main' panicked at tests/ecs/ambiguity_detection.rs:58:5: assertion `left == right` failed: Main app has unexpected ambiguities among these schedules: [Update]. More Details: AmibiguitiesCount( { FixedPostUpdate: 0, FixedLast: 0, StateTransition: 0, RunFixedMainLoop: 0, FixedMain: 0, PostUpdate: 64, FixedFirst: 0, Last: 3, PostStartup: 0, First: 1, Update: 1, SpawnScene: 0, Main: 0, PreStartup: 0, PreUpdate: 13, }, ) left: 1 right: 0

tests/ecs/ambiguity_detection.rs

Vrixyz · 2024-07-17T20:19:36Z

crates/bevy_dev_tools/src/ci_testing/mod.rs

-            .add_systems(Update, systems::send_events);
+            .add_systems(
+                Update,
+                systems::send_events.before(bevy_window::close_when_requested),


This is kinda outside the scope of this PR, and now useless as there has been a new conflict on Update...

janhohenheim

Some more quality suggestions. These should compile if you accept all of them, but I haven't run them locally.

janhohenheim · 2024-07-17T20:26:02Z

tests/ecs/ambiguity_detection.rs

+    for kv in ambiguities.0.iter() {
+        if ignored_schedules.iter().any(|label| **label == **kv.0) {
+            continue;
+        }
+        if *kv.1 != 0 {
+            unexpected_ambiguities.push(kv.0);
+        }
+    }


Suggested change

for kv in ambiguities.0.iter() {

if ignored_schedules.iter().any(|label| **label == **kv.0) {

continue;

}

if *kv.1 != 0 {

unexpected_ambiguities.push(kv.0);

}

}

for (&label, &count) in ambiguities.0.iter() {

if ignored_schedules.contains(&label) {

continue;

}

if count != 0 {

unexpected_ambiguities.push(label);

}

}

The contains() bit does not compile because we have InternedScheduleLabel on one side, and Box<ScheduleLabel> on the other ; I'm not sure how to convert from one to the other.

tests/ecs/ambiguity_detection.rs

Co-authored-by: Jan Hohenheim <[email protected]>

tests/ecs/ambiguity_detection.rs

Co-authored-by: Jan Hohenheim <[email protected]>

alice-i-cecile reviewed Jun 20, 2024

View reviewed changes

examples/ambiguity_detection.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jun 20, 2024

View reviewed changes

examples/ambiguity_detection.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jun 20, 2024

View reviewed changes

Vrixyz commented Jun 21, 2024

View reviewed changes

Vrixyz requested a review from alice-i-cecile June 21, 2024 16:16

hymm reviewed Jun 21, 2024

View reviewed changes

Vrixyz commented Jun 22, 2024

View reviewed changes

examples/ecs/ambiguity_detection.rs Outdated Show resolved Hide resolved

Vrixyz commented Jun 22, 2024

View reviewed changes

examples/ecs/ambiguity_detection.rs Outdated Show resolved Hide resolved

Vrixyz force-pushed the test_ambiguity_systems_order branch from eac2b3f to 6ace0ed Compare June 22, 2024 21:16

janhohenheim added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 23, 2024

alice-i-cecile added this to the 0.15 milestone Jun 25, 2024

This was referenced Jul 7, 2024

System order ambiguities vladbat00/bevy_egui#287

Open

Deny internal execution order ambiguities in CI #7386

Open

AnthonyTornetta mentioned this pull request Jul 10, 2024

Remove ambiguities in system ordering AnthonyTornetta/Cosmos#290

Closed

alice-i-cecile changed the title ~~test for ambiguity system ordering~~ Test for ambiguous system ordering in CI Jul 14, 2024

alice-i-cecile reviewed Jul 14, 2024

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 14, 2024

Vrixyz added 2 commits July 15, 2024 23:35

fix compilation

d7226e6

adapt allowed ambiguities with main

63937a7

Vrixyz force-pushed the test_ambiguity_systems_order branch from 3d57d41 to 63937a7 Compare July 15, 2024 21:43

Vrixyz changed the base branch from release-0.14.0 to main July 15, 2024 21:43

Vrixyz commented Jul 15, 2024

View reviewed changes

Vrixyz closed this Jul 16, 2024

Vrixyz reopened this Jul 16, 2024

Vrixyz added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 16, 2024

alice-i-cecile reviewed Jul 17, 2024

View reviewed changes

alice-i-cecile approved these changes Jul 17, 2024

View reviewed changes

alice-i-cecile requested a review from BD103 July 17, 2024 15:28

janhohenheim approved these changes Jul 17, 2024

View reviewed changes

janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 17, 2024

janhohenheim reviewed Jul 17, 2024

View reviewed changes

tests/ecs/ambiguity_detection.rs Outdated Show resolved Hide resolved

pr feedback: renames + use struct to gather information about conflicts

c42a53d

Vrixyz commented Jul 17, 2024

View reviewed changes

fix wording + test

d031598

janhohenheim approved these changes Jul 17, 2024

View reviewed changes

Vrixyz and others added 2 commits July 17, 2024 22:33

Apply suggestions from code review

7b1a528

Co-authored-by: Jan Hohenheim <[email protected]>

revert a suggestion as does not compile

71f7731

janhohenheim approved these changes Jul 17, 2024

View reviewed changes

tests/ecs/ambiguity_detection.rs Outdated Show resolved Hide resolved

fix typo

c3bdac5

Co-authored-by: Jan Hohenheim <[email protected]>

alice-i-cecile added this pull request to the merge queue Jul 17, 2024

Merged via the queue into bevyengine:main with commit 26fc4c7 Jul 17, 2024
31 checks passed

alice-i-cecile mentioned this pull request Jul 31, 2024

time_system is ambiguous_with event_update_system #14544

Merged

		/// FIXME: bevy should not have any ambiguities, but it takes time to clean these up,
		/// so we're juste ignoring those for now.

		@@ -0,0 +1,78 @@
		//! An example to confirm that `bevy` doesn't have system order ambiguity with [`DefaultPlugins`]
		//! This is run in CI to ensure that this doesn't regress again.

Uh oh!

Test for ambiguous system ordering in CI #13950

Test for ambiguous system ordering in CI #13950

Uh oh!

Conversation

Vrixyz commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Jun 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 22, 2024

Uh oh!

Vrixyz commented Jun 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Vrixyz commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz commented Jul 17, 2024

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

janhohenheim left a comment

Choose a reason for hiding this comment

Uh oh!

janhohenheim Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Vrixyz commented Jun 20, 2024 •

edited

Loading

Vrixyz Jun 21, 2024 •

edited

Loading

Vrixyz Jun 22, 2024 •

edited

Loading

Vrixyz Jul 15, 2024 •

edited

Loading

Vrixyz Jul 15, 2024 •

edited

Loading

Vrixyz Jul 17, 2024 •

edited

Loading

Vrixyz Jul 17, 2024 •

edited

Loading

janhohenheim Jul 17, 2024 •

edited

Loading

Vrixyz Jul 17, 2024 •

edited

Loading