Skip to content
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.

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

Ensure adaptive properties work as expected for SpecCluster #8324

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Nov 2, 2023

The ordering in which we mutate Cluster.workers in Cluster._correct_state_internal has an impact on how the adaptive properties plan, requested, observed behave.

#8233 changed this ordering in a way that is incompatible with the meaning of those properties but apparently this is not tested.

#8314 is proposing to introduce another ordering change that is incompatible with this.

This unit test should guarantee that the definition of those properties is respected.

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       27 files  ±  0         27 suites  ±0   14h 20m 34s ⏱️ - 53m 8s
  3 944 tests +  1    3 825 ✔️ +  4     117 💤 ±0  2  - 3 
49 539 runs  +31  47 170 ✔️ +41  2 367 💤  - 6  2  - 4 

For more details on these failures, see this check.

Results for commit 12835e6. ± Comparison against base commit 76dd800.

Copy link
Contributor

@milesgranger milesgranger left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@Vesyrak
Copy link

Vesyrak commented Nov 13, 2023

Just want to self-bump here, noting that there's an issue with the observed state, as I defined in:

#8119
#8273

@hendrikmakait
Copy link
Member

@fjetter: What's the status here? Do you need me to review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants