You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "abderrahim (via GitHub)" <gi...@apache.org> on 2025/04/05 07:47:49 UTC
[PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
abderrahim opened a new pull request, #2005:
URL: https://github.com/apache/buildstream/pull/2005
For instance, we shouldn't schedule new build jobs when asked to quit. However, not considering the build queue at all would miss pushing newly built elements.
What this commit does is:
* Pull elements forward through all queues (including the ones we no longer need)
* Only schedule jobs after the imperative queue
This means that elements in the build queue (or any imperative queue) will still get passed to the following queues, but no new build jobs get scheduled.
Fixes https://github.com/apache/buildstream/issues/1787
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2926896463
You can also see this in CI (with python 3.13, no less): https://github.com/apache/buildstream/actions/runs/15327164427/job/43124550203?pr=2005#step:3:8187
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#discussion_r2114151169
##########
tests/integration/cachedfail.py:
##########
@@ -514,3 +425,61 @@ def generate_target():
# Assert that it's no longer cached, and returns to a buildable state
assert cli.get_element_state(project, "target.bst") == "buildable"
+
+
+# Test that we do not keep scheduling builds after one build fails
+# with `--builders 1` and `--on-error quit`.
+#
+# Note that this depends on staging order (which is stable and a part of cache key calculation),
Review Comment:
Ok, I've taken your approach of just launching two failing builds, and also verified that the new test also fails with your patch reverted... this one should be good !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2924789588
So the update here is that this fails in CI, and we're confident that the tests are correct.
The original patch to the scheduler appears to be working with python 3.13 but not in the other python versions which are covered in CI. The patch needs to be better understood, clarified and made to work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#discussion_r2113871052
##########
tests/integration/cachedfail.py:
##########
@@ -514,3 +425,61 @@ def generate_target():
# Assert that it's no longer cached, and returns to a buildable state
assert cli.get_element_state(project, "target.bst") == "buildable"
+
+
+# Test that we do not keep scheduling builds after one build fails
+# with `--builders 1` and `--on-error quit`.
+#
+# Note that this depends on staging order (which is stable and a part of cache key calculation),
Review Comment:
> It seems that CI disagrees with this statement :) I also think that the order isn't deterministic, given the scheduler queues and threads and whatnot.
Staging order is definitely deterministic, and it is required to be, because the dependency order is considered in cache key calculation (recently @juergbi fixed an issue with this, due to newer python having different sort techniques).
The build order seems to not follow staging order to the tee, I would've expected it to when specifying `--builders 1` though... lemme look into this :-/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2888803996
Can we add a test here ?
I think that we can potentially test correct behavior, as described in the comments of #1787, by constructing a test which:
* Has an artifactshare
* Has a build which fails
* Test that the failed build exists in the artifact share after the failed `bst build` call
* And/or use the `cli.get_pushed_elements()` checker to also verify the build log
I think we already have a similar test in `tests/integration/cachedfail.py::test_push_cached_fail()`, perhaps this requires constructing a dependency chain which fails the existing test but passes with this change applied ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2888822388
> I think we already have a similar test in `tests/integration/cachedfail.py::test_push_cached_fail()`, perhaps this requires constructing a dependency chain which fails the existing test but passes with this change applied ?
I think there is a misunderstanding here. What this test does (and what makes me confident that my changes don't break anything) is test for correctness "buildstream pushes the artifacts it has built before quitting". While my change is about interactivity "buildstream quits ASAP when asked to".
I don't think I can reliably test for the interactivity part as buildstream isn't deterministic (or at least it isn't guaranteed to be deterministic) in the order in which it schedules ready jobs. How I would test this manually is to have two build jobs that would run in parallel, set --builders to 1, then interrupt the build and request quit during the build of the first element that gets scheduled.
Talking this through, I think we might be able to do the same thing with failed builds: have two elements that fail, launch the build with `--on-error quit`, and assert that only one of two elements gets built and pushed. I'll give it a go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2888833487
> Talking this through, I think we might be able to do the same thing with failed builds: have two elements that fail, launch the build with `--on-error quit`, and assert that only one of two elements gets built and pushed. I'll give it a go.
Thats what I was thinking (at least the `--on-error quit` part), also, to control the behavior, we want an explicit `--builders`, probably `--builders 1` to constrain buildstream to build/fail only one element at a time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#discussion_r2109263715
##########
tests/integration/cachedfail.py:
##########
@@ -514,3 +425,61 @@ def generate_target():
# Assert that it's no longer cached, and returns to a buildable state
assert cli.get_element_state(project, "target.bst") == "buildable"
+
+
+# Test that we do not keep scheduling builds after one build fails
+# with `--builders 1` and `--on-error quit`.
+#
+# Note that this depends on staging order (which is stable and a part of cache key calculation),
Review Comment:
It seems that CI disagrees with this statement :) I also think that the order isn't deterministic, given the scheduler queues and threads and whatnot.
The way I was thinking about this is to have two failing builds and assert that one is "failed" and one is "buildable" (not necessarily in this order)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2926853587
I think it's not fair to say that this is dependent on the python version. It's not fair to say it's because of this patch either: it could be a race condition somewhere else in the scheduler that we only see now with this test (and God knows how many race conditions there are in the scheduler, I remember two manifestations of such race conditions in everyday usage of buildstream).
As a data point, I just did 10 back-to-back runs of just these two tests (`test_stop_building_after_failed` and `test_push_but_stop_building_after_failed`) and all of them succeeded on all supported python versions.
I know this failure exists, and could reproduce it on my laptop at least once, but this is definitely some non-deterministic behaviour rather than a real issue with the patch.
I would recommend landing this patch now: it's definitely better than what we currently have, and we can keep investigating the test failure and land the test later (along with any fix to make it always pass)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan merged PR #2005:
URL: https://github.com/apache/buildstream/pull/2005
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#discussion_r2094049650
##########
src/buildstream/_scheduler/scheduler.py:
##########
@@ -371,7 +371,7 @@ def _sched_queue_jobs(self):
# Pull elements forward through queues
elements = []
- for queue in queues:
+ for queue in self.queues:
Review Comment:
> The first part of the function's job is to select the queues on which to process in the latter half of the function, that seems to make sense. And except for this line you've changed, the rest of the function does only operate on the selected queues.
Correct, and that's the root cause of the issue I'm trying to fix.
There are two operations in this function ("pull elements forward through queues", and "kickoff whatever processes can be processed at this time") and they shouldn't operate on the same list of queues.
Let's consider a simple example where we have three queues: fetch, build and push. Once we receive the quit signal, we only want to:
* pull elements forward from build to push
* kickoff new jobs in push
What the current code does is:
* pull elements forward from build to push
* kickoff new jobs in build (<- not desired, and is the issue I'm trying to fix)
* kickoff new jobs in push
What the new code does is:
* pull elements forward from fetch to build (<- undesired but harmless, this is the issue you're pointing)
* pull elements through from build to push
* kickoff new jobs in push
I hope this explains it. So while this line is indeed "dubious", it needs to be different from the other one. And since it's harmless to pull jobs into next queues even when asked to quit, I think it's fine to do that unconditionally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2903356191
[...]
>
> Talking this through, I think we might be able to do the same thing with failed builds: have two elements that fail, launch the build with `--on-error quit`, and assert that only one of two elements gets built and pushed. I'll give it a go.
I've rebased your branch, and added:
* A refactor of `tests/integration/cachedfail.py`, so that it doesn't share the `project/` directory with other integration tests
* Added regression tests for #1787
I've verified that these new tests indeed fail with your changes reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#issuecomment-2926881762
I just reproduced the failure locally, and looking at the log I can see that the issue is that the cache isn't correctly cleaned up between the tests.
At the pipeline summary when buildstream starts, one of the two elements is already "failed" (and base is already "cached").
```
Pipeline
cached 32de3a73cff66516cefb7922fe38627bd66eb08de11bc3de55f717e9abd06e01 base.bst
buildable 326d9b9f2c93d81bd2fd53a19bd4fb310f4223f8ab8fb07c1e14dc386b06cad7 base-also-fail.bst
failed af38817f3fa7ade48339e6891a33972dedaadcabc73510925a53b5670ace64f2 base-fail.bst
waiting ab71767c47a2b0e12eafbf28594a73e5a9ad66c05554c88ac1b5cb5aeacb2240 depends-on-two-failures.bst
```
BuildStream then builds the other one and has two failures at the end.
In the second test, both files were already cached failures before the build started.
```
Pipeline
cached 32de3a73cff66516cefb7922fe38627bd66eb08de11bc3de55f717e9abd06e01 base.bst
failed 326d9b9f2c93d81bd2fd53a19bd4fb310f4223f8ab8fb07c1e14dc386b06cad7 base-also-fail.bst
failed af38817f3fa7ade48339e6891a33972dedaadcabc73510925a53b5670ace64f2 base-fail.bst
waiting ab71767c47a2b0e12eafbf28594a73e5a9ad66c05554c88ac1b5cb5aeacb2240 depends-on-two-failures.bst
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] scheduler: Stop scheduling from the imperative queue when asked to quit [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #2005:
URL: https://github.com/apache/buildstream/pull/2005#discussion_r2094397674
##########
src/buildstream/_scheduler/scheduler.py:
##########
@@ -371,7 +371,7 @@ def _sched_queue_jobs(self):
# Pull elements forward through queues
elements = []
- for queue in queues:
+ for queue in self.queues:
Review Comment:
Ok... I've done some mental gymnastics and I agree with your statement :)
What I would like to see here is:
* Rename the local `queues` variable to something more explicit, perhaps `queues_to_process`
* At least slightly elaborate on the comment *"Pull elements forward through queues"*
* Perhaps something like *"Pull elements forward through all queues, regardless of whether we are processing those queues"*
* Maybe even a little explanation, like *"We need to propagate elements through all queues even if we are not processing post-imperative queues, so that we do end up processing the jobs we need to"*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org