Skip to content

[core][dashboard] Change py-spy native default to false#30636

Merged
ericl merged 5 commits intoray-project:masterfrom
rickyyx:profile-default
Nov 28, 2022
Merged

[core][dashboard] Change py-spy native default to false#30636
ericl merged 5 commits intoray-project:masterfrom
rickyyx:profile-default

Conversation

@rickyyx
Copy link
Copy Markdown
Member

@rickyyx rickyyx commented Nov 24, 2022 •
edited
Loading

Signed-off-by: rickyyx rickyx@anyscale.com

Why are these changes needed?

Related issue number

Closes #30566

Checks

Run agent test which failed to get profiling/stacktrace now runs OK:
image

Changing query to native=1 will fail the query again:
image

  • 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 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 :(

@rickyyx
default native false
431cc4c 
Signed-off-by: rickyyx <rickyx@anyscale.com>
ericl
ericl reviewed Nov 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

Can we add native=0 to the href as well in the frontend? This is to surface that you can set native=1 to turn it on.

@rickyyx
surface native default = 0 on frontend
4545c0f 
Signed-off-by: rickyyx <rickyx@anyscale.com>
ericl
ericl reviewed Nov 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

I think you'll also need to add/fix the test in test_dashboard_profiler.py, which assumes native frames right now.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 24, 2022
@rickyyx
Copy link
Copy Markdown
Member Author

rickyyx commented Nov 24, 2022

I think you'll also need to add/fix the test in test_dashboard_profiler.py, which assumes native frames right now.

Oh I see, had some troubles running the test locally. Will take a look at the CI run.

rkooo567
rkooo567 reviewed Nov 24, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Is it possible to test the change?

@ericl
Copy link
Copy Markdown
Contributor

ericl commented Nov 24, 2022

Yes, we should add a test case to the profiler suite to test the flag.

@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 Nov 28, 2022
@rickyyx
Copy link
Copy Markdown
Member Author

rickyyx commented Nov 28, 2022

Modified the test to test native vs non-native profiling.

Wondering if there's any good way for us to let users know the default is non-native, and they could change the parameter for native profiling?

Have we thought about default to native, and retry with non native if the native profiling fails?

ericl
ericl reviewed Nov 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

It's not a bad idea. Like with native=1, try-catch native and fallback to non-native? Then we could enable native by default.

Not sure if there are any downsides here.

ericl
ericl reviewed Nov 28, 2022
View reviewed changes
@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 28, 2022
@rickyyx
Copy link
Copy Markdown
Member Author

rickyyx commented Nov 28, 2022 •
edited
Loading

Like with native=1, try-catch native and fallback to non-native? Then we could enable native by default.

Right - we could probably hide the option from url? If native=1 in url, but then we not showing the native due to fallback, it might be a bit confusing?

@ericl
Copy link
Copy Markdown
Contributor

ericl commented Nov 28, 2022

Right - we could probably hide the option from url? If native=1 in url, but then we not showing the native due to fallback, it might be a bit confusing?

Maybe let's do this in a separate PR then, to keep the cherry pick small / uncontroversial.

@rickyyx
explicit assert for non-native mode
7b1d26c 
Signed-off-by: Ricky Xu <xuchen727@hotmail.com>
@rickyyx rickyyx added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Nov 28, 2022
@ericl ericl merged commit 79783bd into ray-project:master Nov 28, 2022
rickyyx added a commit to rickyyx/ray that referenced this pull request Nov 28, 2022
@rickyyx
[core][dashboard] Change py-spy native default to false (ray-project#…
0c313a9 
…30636)
ericl pushed a commit that referenced this pull request Nov 28, 2022
@rickyyx
[pick] [core][dashboard] Change py-spy native default to false (#30636)…
2fc186d 
… (#30712)
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
@rickyyx @WeichenXu123
[core][dashboard] Change py-spy native default to false (ray-project#…
750bdda 
…30636)

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
@rickyyx @tamohannes
[core][dashboard] Change py-spy native default to false (ray-project#…
102ede6 
…30636)

Signed-off-by: tmynn <hovhannes.tamoyan@gmail.com>
@andrew-anyscale andrew-anyscale mentioned this pull request Mar 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericl ericl ericl approved these changes

@rkooo567 rkooo567 rkooo567 left review comments

Assignees

@ericl ericl

@rkooo567 rkooo567

Labels

tests-ok The tagger certifies test failures are unrelated and assumes personal liability.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Dashboard] Getting stacktrace from the dashboard triggers "failed to get os threadid" when --native option is used

3 participants

@rickyyx @ericl @rkooo567