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

[ONNXTransform] always delete orphaned node branches after running an onnx transform #1746

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

bfineran
Copy link
Member

@bfineran bfineran commented Oct 3, 2023

adds a pass in the base ONNXTransform cleanup steps to run ONNXGraph.delete_orphaned_node_branches which is an important additional cleanup step. This removes unwanted nodes from remaining in the graph after a transform which could lead to issues at inference time. This function should be safe to run against all transforms and only adds minimal overhead (O(nodes^2)) if any orphaned nodes are found

should fix issues in KVCache injected MPT models which seem to consistently get shipped with orphaned nodes remaining

test_plan:
verified locally that this unblocked running a pipeline with zoo:nlg/text_generation/mpt-7b/pytorch/huggingface/mpt_chat/base-none

future testing should pull down a model with no data files and test injection against it

@bfineran bfineran self-assigned this Oct 3, 2023
anmarques
anmarques previously approved these changes Oct 3, 2023
Copy link
Member

@anmarques anmarques left a comment

Choose a reason for hiding this comment

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

Tested on mpt-7b-mpt_pretrain-base and it worked to produce a valid model

@bfineran
Copy link
Member Author

bfineran commented Oct 3, 2023

tests fixed, merging

@bfineran bfineran merged commit 2d0f8a0 into main Oct 3, 2023
11 checks passed
@bfineran bfineran deleted the always-delete-orphaned-nodes branch October 3, 2023 19:56
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.

None yet

2 participants