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

[core][state] Case insensitive match for string value for state API filter. #34577

Merged
merged 7 commits into from
Aug 29, 2023

Conversation

rickyyx
Copy link
Contributor

@rickyyx rickyyx commented Apr 19, 2023

Why are these changes needed?

Ran into an unexpected failure on #34554, where the query changes as we changes one of the value from (Worker to WORKER)

I think state API filtering could be less strict about case sensitivity, and I feel loosing it altogether would be ok and more usable.

I can hardly think of a usecase where case sensitivity on some fields would be useful. Of course, we could add a flag to the API to toggle this behaviour.

Related issue number

Closes #34554

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: rickyyx <rickyx@anyscale.com>
Signed-off-by: rickyyx <rickyx@anyscale.com>
Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

can you update the docstring it is case insensitive? (the CLI docstring)

@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Apr 19, 2023
Signed-off-by: Ricky Xu <xuchen727@hotmail.com>
Signed-off-by: Ricky Xu <xuchen727@hotmail.com>
@stale
Copy link

stale bot commented Jun 10, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 10, 2023
@rickyyx
Copy link
Contributor Author

rickyyx commented Jun 12, 2023

TODO

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 12, 2023
@stale
Copy link

stale bot commented Jul 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jul 15, 2023
@rickyyx
Copy link
Contributor Author

rickyyx commented Jul 17, 2023

Oops this slipped from the GA - @rkooo567 but I feel we could still get this in? Maybe add a change log in the docstring to document this like python does for each function.

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jul 17, 2023
@rkooo567
Copy link
Contributor

yeah I think this makes more sense!

@rkooo567
Copy link
Contributor

merge conflict?

Signed-off-by: rickyyx <rickyx@anyscale.com>
Signed-off-by: rickyyx <rickyx@anyscale.com>
@rickyyx rickyyx removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2023
@rickyyx
Copy link
Contributor Author

rickyyx commented Aug 25, 2023

A usability improvement for state API, would be great to get this in. cc @zhe-thoughts

TODO: waiting for test results. Adding test-ok once ready.

@zhe-thoughts zhe-thoughts removed their assignment Aug 28, 2023
@zhe-thoughts
Copy link
Collaborator

@rickyyx @rkooo567 Please get this merged into master first (master is unfrozen for now). Thanks

@rickyyx
Copy link
Contributor Author

rickyyx commented Aug 29, 2023

Test failures unrelated.

@rickyyx rickyyx merged commit 3bed30b into ray-project:master Aug 29, 2023
88 of 94 checks passed
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
…ilter. (ray-project#34577)

---------

Signed-off-by: Ricky Xu <xuchen727@hotmail.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
LeonLuttenberger pushed a commit to jaidisido/ray that referenced this pull request Sep 5, 2023
…ilter. (ray-project#34577)


---------


Signed-off-by: Ricky Xu <xuchen727@hotmail.com>
jimthompson5802 pushed a commit to jimthompson5802/ray that referenced this pull request Sep 12, 2023
…ilter. (ray-project#34577)

---------

Signed-off-by: Ricky Xu <xuchen727@hotmail.com>
Signed-off-by: Jim Thompson <jimthompson5802@gmail.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
…ilter. (ray-project#34577)

---------

Signed-off-by: Ricky Xu <xuchen727@hotmail.com>
Signed-off-by: Victor <vctr.y.m@example.com>
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.

[core][ci] stress_test_state_api_scale.aws fails
3 participants