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.

Sign up for GitHub

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

Jump to bottom

[core][compiled graphs] Rework visualize with channel info and actor coloring #48473

Merged
merged 17 commits into from
Nov 15, 2024
Merged

[core][compiled graphs] Rework visualize with channel info and actor coloring #48473

merged 17 commits into from
Nov 15, 2024

Conversation

dayshah
Copy link
Contributor

@dayshah dayshah commented Oct 31, 2024 •
edited
Loading

Why are these changes needed?

To give an option for additional channel information for debugging using the new dag visualization functionality.

After:

Before:

Related issue number

Closes #48248

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

@dayshah
Add extra channel info to visualization
3ae8a22 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah
inner channel num readers for cached channel
1b9dccc 
Signed-off-by: dayshah <dhyey2019@gmail.com>
dayshah
dayshah commented Oct 31, 2024
View reviewed changes
python/ray/experimental/channel/cached_channel.py Outdated
"""
Return the number of readers for this channel.
"""
return self._inner_channel.num_readers() if self._inner_channel else 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there an alternative we should be going to here if the inner channel doesn't exist?

@dayshah dayshah requested a review from ruisearch42 October 31, 2024 17:26
@jjyao jjyao added the go add ONLY when ready to merge, run all tests label Nov 1, 2024
@dayshah
Merge remote-tracking branch 'origin/master' into dag-channel-viz
76559b3 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah
rework drawing graph through downstream and remove num_readers
c02fe8e 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah
channels on edges
b4d77c8 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah dayshah marked this pull request as ready for review November 6, 2024 16:40
@ruisearch42
Copy link
Contributor

@dayshah Is this ready to review?

@dayshah
Copy link
Contributor Author

dayshah commented Nov 6, 2024

@dayshah Is this ready to review?

ya

@dayshah dayshah changed the title [core][compiled graphs]Add extra channel info to visualization [core][compiled graphs] Rework visualize to add extra channel info Nov 6, 2024
@dayshah
remove extra param
b423c72 
Signed-off-by: dayshah <dhyey2019@gmail.com>
ruisearch42
ruisearch42 reviewed Nov 6, 2024
View reviewed changes
Copy link
Contributor

@ruisearch42 ruisearch42 left a comment

Choose a reason for hiding this comment

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

This is a good first step.

In the description, can you show the old visualization as well so the difference will be obvious.

python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
python/ray/dag/compiled_dag_node.py Show resolved Hide resolved
python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
@dayshah
address comments
7132907 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah dayshah requested a review from ruisearch42 November 6, 2024 22:47
ruisearch42
ruisearch42 reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@ruisearch42 ruisearch42 left a comment

Choose a reason for hiding this comment

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

Do you still need to update the example graphs or code? I see some channels are still showing CompositeChannel

python/ray/dag/compiled_dag_node.py Show resolved Hide resolved
python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
@dayshah
update channel_detail arg stuff
65db0d4 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah
update get output channel type
0695858 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah
Copy link
Contributor Author

dayshah commented Nov 7, 2024

Do you still need to update the example graphs or code? I see some channels are still showing CompositeChannel

So it seems like there's actually only one cached channel i printed out all the channels in the channel dict to make sure too and it showed only one cached channel that leads to the two tasks

@dayshah dayshah requested a review from ruisearch42 November 7, 2024 17:03
ruisearch42
ruisearch42 reviewed Nov 8, 2024
View reviewed changes
python/ray/dag/compiled_dag_node.py Outdated
if dag_node.type_hint
else "UnknownType"
) + "\n"
get_output_channel_type = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add some unit tests in

ray/python/ray/dag/tests/experimental/test_dag_visualization.py

Line 11 in 871d4d0

def test_visualize_basic(ray_start_regular):

@dayshah
color based on actor and more channel info
e282908 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah dayshah requested a review from ruisearch42 November 12, 2024 17:11
@dayshah
edit color actor eq
60ae7b2 
Signed-off-by: dayshah <dhyey2019@gmail.com>
ruisearch42
ruisearch42 reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@ruisearch42 ruisearch42 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the coloring!

python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
ruisearch42
ruisearch42 reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@ruisearch42 ruisearch42 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 also add a test for multi-arg and kwarg and make sure it works OK? They will be created as InputAttributeNodes and a channel is created for each InputAttributeNode. See how they can be created here:

ray/python/ray/dag/tests/experimental/test_accelerated_dag.py

Line 652 in 37709da

class TestMultiArgs:

After addressing these we should be able to merge!

@dayshah
add id for buffered shared memory
a7d8fd9 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah
Copy link
Contributor Author

dayshah commented Nov 12, 2024

Can you also add a test for multi-arg and kwarg and make sure it works OK? They will be created as InputAttributeNodes and a channel is created for each InputAttributeNode. See how they can be created here:

ray/python/ray/dag/tests/experimental/test_accelerated_dag.py

Line 652 in 37709da

class TestMultiArgs:

After addressing these we should be able to merge!

added multiarg test and image of multiarg kwarg visualization in description

@dayshah dayshah requested a review from ruisearch42 November 12, 2024 20:12
@dayshah dayshah changed the title [core][compiled graphs] Rework visualize to add extra channel info [core][compiled graphs] Rework visualize with channel info and actor coloring Nov 12, 2024
ruisearch42
ruisearch42 approved these changes Nov 14, 2024
View reviewed changes
python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
python/ray/dag/compiled_dag_node.py Outdated
@@ -2281,11 +2298,13 @@ def visualize(

# Dot file for debuging
dot = graphviz.Digraph(name="compiled_graph", format=format)

# Give every actor a unique color (Colors between 24k -> 40k seem readable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you imply showing up to 16 colors should be OK, but the coloring scheme does not scale well beyond that? If so, please add that to the comment explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the coloring scheme will keep going infinitely and wrap around, but the text is always black, and the colors could end up being too dark so the text isn't as visible, will add that info in comment

python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
python/ray/dag/compiled_dag_node.py Outdated
else "UnknownType"
) + "\n"

def get_output_channel_info(output_channel, downstream_actor_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this out of the for loop. Also add docstring.

Let's rename the method to get_channel_details. Let's just call the arg channel as opposed to output_channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just moved it outside the function in general, can be called for debugging purposes on its own

python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
python/ray/experimental/channel/shared_memory_channel.py Outdated Show resolved Hide resolved
@dayshah
address comments
b9d490b 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah
extract get_channel_details to its own func
1092405 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@ruisearch42
Copy link
Contributor

ruisearch42 commented Nov 14, 2024 •
edited
Loading

test_accelerated_dag failed, please address

ruisearch42
ruisearch42 approved these changes Nov 14, 2024
View reviewed changes
python/ray/dag/compiled_dag_node.py Outdated Show resolved Hide resolved
@dayshah
add type to get_channel_details
3841cf8 
Signed-off-by: dayshah <dhyey2019@gmail.com>
@jjyao jjyao merged commit 42d101e into ray-project:master Nov 15, 2024
5 checks passed
@dayshah dayshah deleted the dag-channel-viz branch November 15, 2024 06:21
dentiny pushed a commit to dentiny/ray that referenced this pull request Dec 7, 2024
@dayshah @dentiny
[core][compiled graphs] Rework visualize with channel info and actor …
b503593 
…coloring (ray-project#48473)

Signed-off-by: dayshah <dhyey2019@gmail.com>
Signed-off-by: hjiang <dentinyhao@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruisearch42 ruisearch42 ruisearch42 approved these changes

Assignees

@ruisearch42 ruisearch42

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[core][compiled graph] Visualize Compiled Graph Channels
3 participants
@dayshah @ruisearch42 @jjyao