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

Replace toposort with graphlib #1942

Merged
merged 23 commits into from
Jul 4, 2024
Merged

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Jun 12, 2024

Description

Related to: #1820

This ticket aims at dropping toposort dependency in favor of built-in graphlib if possible.

Development notes

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@SajidAlamQB SajidAlamQB self-assigned this Jun 12, 2024
@SajidAlamQB SajidAlamQB changed the title [DRAFT] Replace toposort with graphlib Replace toposort with graphlib Jun 24, 2024
@SajidAlamQB SajidAlamQB marked this pull request as ready for review June 26, 2024 11:37
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM, and the tests are all passing.

So Toposort performs several tasks under the hood, such as determining the layer and parent layer relationships. But, with graphlib, we must explicitly ensure that we don't add a layer as its own parent. Interesting.

@ravi-kumar-pilla
Copy link
Contributor

Hi @SajidAlamQB ,

Nicely done. I was testing the PR and I see there is a minor issue in the UI. The order of layers is shifted -

Layers feature and model_input are shifted. Even the DAG has Feature Importance Output dataset after Model Input Table. Can you please test it on your side. Thank you

Before

image

After

image

@SajidAlamQB
Copy link
Contributor Author

Layers feature and model_input are shifted. Even the DAG has Feature Importance Output dataset after Model Input Table. Can you please test it on your side. Thank you

Hey @ravi-kumar-pilla, this should be fixed now let me know if its all fine.

@ravi-kumar-pilla
Copy link
Contributor

Hey @ravi-kumar-pilla, this should be fixed now let me know if its all fine.

Hi @SajidAlamQB , I tested it now and the order looks good. Thank you for that. May I know why was the static order different or why - return sorted(sorted_layers, key=lambda layer: (sorted_layers.index(layer), layer)) was needed.

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

LGTM ! It would be nice to have this in the release note to track the changes. Thank you @SajidAlamQB

@SajidAlamQB
Copy link
Contributor Author

Hey @ravi-kumar-pilla, this should be fixed now let me know if its all fine.

Hi @SajidAlamQB , I tested it now and the order looks good. Thank you for that. May I know why was the static order different or why - return sorted(sorted_layers, key=lambda layer: (sorted_layers.index(layer), layer)) was needed.

The order of layers by the TopologicalSorter was decided by their insertion order. Since the insertion order was not stable it led to unexpected ordering in the output. So adding that final sort helped maintain the original input order. I will add some comments here for future reference as well.

@SajidAlamQB SajidAlamQB requested a review from astrojuanlu as a code owner July 3, 2024 15:40
Signed-off-by: Sajid Alam <[email protected]>
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

LGTM! 👏🏼

@SajidAlamQB SajidAlamQB merged commit cddd25c into main Jul 4, 2024
39 checks passed
@SajidAlamQB SajidAlamQB deleted the replace-toposort-with-graphlib branch July 4, 2024 09:00
@SajidAlamQB SajidAlamQB mentioned this pull request Jul 25, 2024
5 tasks
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