[no_early_kickoff][core][dashboard] Feature flag task logs recording (#34056) #34101
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
This should address a few issues introduced by the original task log recording features: [core] perf regression: 1_1_actor_calls_concurrent ray-project#33924 [core] perf regression: 1_1_actor_calls_async ray-project#33949 [Tests] Fix two skipped Windows test for test_task_event_2.py ray-project#33738 The roocasue with the regressions are: With ray-project#32943, we are recording log file offsets before and after executing a task, which calls tell() on the file descriptor object for each worker. The cost of that shows up when there are concurrent execution of tasks on a single worker. I am turning this off by default for this release since the subsequent PRs are not merged yet. We will need to tackle or resolve the regression once we turn this feature on when we merge subsequent PRs. One idea is to make this "finding-out-offset-procedure" async, e.g. we try to locate the exact task id's log offset when we querying the task logs at querying time.
|
Approval here: #34056 (review) |
clarng
approved these changes
Apr 5, 2023
|
fix dco? |
|
Fixed. |
|
The workflow test failure looks new 2.4.0 doesn't have it : https://buildkite.com/ray-project/oss-ci-build-branch/builds/3201 |
|
oh which job is that? It might be flaky. |
https://buildkite.com/ray-project/oss-ci-build-pr/builds/17527#01875261-4ea5-41df-98ba-6e5a80845dfd |
|
The test looks unrelated (workflow logging is completely different from this I believe). Let me retry first. |
…-exp-log-regression
|
Hmm looks like it is still failing |
|
Hmm, that's surprising. Let me look into it more closely. |
Signed-off-by: rickyyx <[email protected]>
rickyyx
requested review from
ericl,
fishbone,
stephanie-wang and
suquark
as code owners
Signed-off-by: rickyyx <[email protected]>
fishbone
approved these changes
Apr 7, 2023
Signed-off-by: rickyyx <[email protected]>
rickyyx
changed the title
Signed-off-by: rickyyx <[email protected]>
rickyyx
added
the
@author-action-required
|
Ok - looks [no_early_kickoff] solves the issue. Must be some incompatability with the docker prebuilt on the release branch. |
Signed-off-by: rickyyx <[email protected]>
rickyyx
removed
the
@author-action-required
|
This should be ready to merge: cc @clarng |
This was referenced Apr 10, 2023
zhe-thoughts
approved these changes
Apr 10, 2023
fishbone
pushed a commit
that referenced
this pull request
Apr 10, 2023
Signed-off-by: rickyyx <[email protected]> We have seen workflow test fails on PRs with totally unrelated content because of the re-using of the cached docker image. - #34101 Seems the workflow test does have a dependency on the wheels built.
elliottower
pushed a commit
to elliottower/ray
that referenced
this pull request
Apr 22, 2023
Signed-off-by: rickyyx <[email protected]> We have seen workflow test fails on PRs with totally unrelated content because of the re-using of the cached docker image. - ray-project#34101 Seems the workflow test does have a dependency on the wheels built. Signed-off-by: elliottower <[email protected]>
ProjectsByJackHe
pushed a commit
to ProjectsByJackHe/ray
that referenced
this pull request
May 4, 2023
Signed-off-by: rickyyx <[email protected]> We have seen workflow test fails on PRs with totally unrelated content because of the re-using of the cached docker image. - ray-project#34101 Seems the workflow test does have a dependency on the wheels built. Signed-off-by: Jack He <[email protected]>
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.
This should address a few issues introduced by the original task log recording features:
[core] perf regression: 1_1_actor_calls_concurrent #33924 [core] perf regression: 1_1_actor_calls_async #33949 [Tests] Fix two skipped Windows test for test_task_event_2.py #33738 The roocasue with the regressions are:
With #32943, we are recording log file offsets before and after executing a task, which calls tell() on the file descriptor object for each worker.
The cost of that shows up when there are concurrent execution of tasks on a single worker.
I am turning this off by default for this release since the subsequent PRs are not merged yet. We will need to tackle or resolve the regression once we turn this feature on when we merge subsequent PRs. One idea is to make this "finding-out-offset-procedure" async, e.g. we try to locate the exact task id's log offset when we querying the task logs at querying time.
Why are these changes needed?
Related issue number
Closes #33924
Closes #33949
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.