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

Fix tag being undefined bug from the backend. #2162

Merged
merged 9 commits into from
Nov 1, 2024

Conversation

rashidakanchwala
Copy link
Contributor

@rashidakanchwala rashidakanchwala commented Oct 29, 2024

Description

Resolves #2106.

Upon investigation based on what user did, I realised that there might be cases where tags might be iteratively added to a node as we iterate through different pipelines. In this case, the node initially had certain tags, but __default__ pipeline later added additional ones. In our GraphNodes repository, once a graph node is defined, it doesn't get updated with new tags.

This PR changes that by allowing nodes to update with any new tags even if they’re already defined. Before the namespace refactor, this issue surprisingly didn’t arise, likely because certain aspects of the refactor introduced unintended behavior that ended up compensating for each other.

See the example to understand better - the code below works fine because __default__ is defined first, so all nodes belonging to the default pipeline are added to GraphNodeRepository with the correct tags, without any errors:

working_pipes = {
    "__default__": pipeline(
        ingestion_pipeline + feature_pipeline + modelling_pipeline + reporting_pipeline,
        tags=["default_tag1", "default_tag2"]
    ),
    "Data ingestion": ingestion_pipeline,
    "Modelling stage": modelling_pipeline,
    "Feature engineering": feature_pipeline,
    "Reporting stage": reporting_pipeline,
    "Pre-modelling": ingestion_pipeline + feature_pipeline,
}
return working_pipes

However, if __default__ is defined last, nodes from ingestion_pipeline are already created. When we try to recreate these nodes with the new tags from __default__, the tags aren’t added since the nodes already exist.

working_pipes = {
    "Data ingestion": ingestion_pipeline,
    "Modelling stage": modelling_pipeline,
    "Feature engineering": feature_pipeline,
    "Reporting stage": reporting_pipeline,
    "Pre-modelling": ingestion_pipeline + feature_pipeline,
    "__default__": pipeline(
        ingestion_pipeline + feature_pipeline + modelling_pipeline + reporting_pipeline,
        tags=["default_tag1", "default_tag2"]
    ),
}
return working_pipes

This PR resolves the issue by checking if a node already exists and, if so, assigning it any additional tags:, existing nodes correctly receive any additional tags defined in subsequent pipelines, preventing skipped tag assignments.

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

Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
@rashidakanchwala rashidakanchwala marked this pull request as ready for review October 30, 2024 11:10
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
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.

Tested the usecase and it looks fixed. Thank you @rashidakanchwala

Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

Thank you @rashidakanchwala!

rashidakanchwala and others added 3 commits November 1, 2024 09:50
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
@rashidakanchwala rashidakanchwala merged commit c7cdab9 into main Nov 1, 2024
33 checks passed
@rashidakanchwala rashidakanchwala deleted the fix-tag-issue-in-be branch November 1, 2024 12:40
@Huongg Huongg mentioned this pull request Nov 21, 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.

Pipelines rendering breaks with exception, ver. 9.2.0, 10.0, yet works on 9.1.0
3 participants