-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[ADAG] Add visualization of compiled graphs #47958
Conversation
cc @kevin85421 can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
@ruisearch42 is this different from the visualization function in your overlap PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After addressing these comments, I think we are good to merge!
Over time we can always improve and add more features. This will be very useful.
This is to visualize the DAG itself, the overlap PR visualizes the schedule. Both will be useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DAG (non-compiled) seems to have visualization support: https://github.com/ray-project/ray/blob/master/python/ray/dag/vis_utils.py.
What's the difference between the existing one and this PR?
Hi, @ruisearch42 , I just update it and solved you comments. The new plots is above with different color and shape for different node. The tests are also included. |
I suspect that graphviz is not available on the test environment so we need to |
@Bye-legumes maybe you can try to use runtime environment. Btw, would you mind answering my question here: #47958 (review)? Thanks! |
This is for the complied graph with different nodes that we have different node handling for different node for complied graph. Also the data transport method is also included as we have hint. This is the result of that plot. How I can set runtime environment for this test case? Should install packages in the test.py with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are test failures for this file. Please fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that graphviz is not available on the test environment so we need to pip install graphviz
and sudo apt-get install graphviz
for visualization test. Do you know how we can achieve that in the test environments? I need to add dependency to which file for pip and apt-get? @ruisearch42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can introduce something like a test requirement.txt , something like python/requirements/ml/data-test-requirements.txt
If that is too complex, we can skip the test for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just skip these tests and I think it's OK now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change!
@Bye-legumes Auto merge is enabled, but tests are failing, please fix |
I see, thx you keep the eye on this! I think this tests failed is not related as other PR also failed on same test. Let me merge the master and check if it will recurs! |
@ruisearch42 Hi, I just merged the master and it's OK now! |
@rkooo567 to merge |
In #47958 , visualization unit tests were introduced but would not run if CI environment does not have graphvis installed (which was the case). This PR adds the dependency in test-requirements.txt and always run the visualization tests. It also moves visualization tests to a separate file since test_accelerated_dag.py is too large.
Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
In ray-project#47958 , visualization unit tests were introduced but would not run if CI environment does not have graphvis installed (which was the case). This PR adds the dependency in test-requirements.txt and always run the visualization tests. It also moves visualization tests to a separate file since test_accelerated_dag.py is too large. Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>
In ray-project#47958 , visualization unit tests were introduced but would not run if CI environment does not have graphvis installed (which was the case). This PR adds the dependency in test-requirements.txt and always run the visualization tests. It also moves visualization tests to a separate file since test_accelerated_dag.py is too large. Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>
Why are these changes needed?
#47945
My test case
plot
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.