Skip to content

[Data] Partial fix for Dataset.context not being sealed after creation#41569

Merged
raulchen merged 2 commits intoray-project:masterfrom
raulchen:fix-plan-copy-context
Dec 4, 2023
Merged

[Data] Partial fix for Dataset.context not being sealed after creation#41569
raulchen merged 2 commits intoray-project:masterfrom
raulchen:fix-plan-copy-context

Conversation

@raulchen
Copy link
Contributor

@raulchen raulchen commented Dec 1, 2023

Why are these changes needed?

Dataset.context should be sealed the first time the Dataset is created. But if a new operator is applied to the dataset, the new global DataContext will be saved again to the Dataset.

This bug prevents using different DataContexts for training and validation datasets in a training job.

Note this PR only fixes the issue when multiple datasets are created in the process but will be running in different processes. If they run in the same process, it's still a bug, see #41573.

Related issue number

#41573

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Hao Chen <chenh1024@gmail.com>
Signed-off-by: Hao Chen <chenh1024@gmail.com>
ds2.take_all()


def test_streaming_split_with_custom_data_context(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this test to test_context_propagation, not changing the test code.

@raulchen raulchen assigned raulchen, stephanie-wang and c21 and unassigned raulchen Dec 1, 2023
@stephanie-wang
Copy link
Contributor

What about the places where we use DataContext.get_current() during planning, e.g., here? Don't we need to propagate the DataContext through to those?

@stephanie-wang stephanie-wang added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 1, 2023
@raulchen
Copy link
Contributor Author

raulchen commented Dec 2, 2023

What about the places where we use DataContext.get_current() during planning, e.g., here? Don't we need to propagate the DataContext through to those?

Good point. So this PR can fix the case for training jobs, where be different datasets will be proposed to different processes (the SplitCoordinator actors) for execution.
But if they run in the driver process, it's still a bug. I created an issue #41573 for tracking.

Copy link
Contributor

@stephanie-wang stephanie-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but can you update the PR description to make it clear what cases this does and does not cover?

@raulchen raulchen changed the title [Data] Fix Dataset.context not being sealed after creation [Data] Partial fix for Dataset.context not being sealed after creation Dec 4, 2023
@raulchen raulchen merged commit 1e691f0 into ray-project:master Dec 4, 2023
@raulchen raulchen deleted the fix-plan-copy-context branch December 4, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants