Skip to content

Conversation

@alanwaketan
Copy link
Collaborator

Summary:
This patch tries to add a test case for BuildInputOutputAliases and fixes the corresponding metric which reports a bogus value that's the total number of outputs.

Test Plan:
PJRT_DEVICE=CPU python test/test_input_output_aliases.py

@alanwaketan alanwaketan self-assigned this Dec 14, 2022
@alanwaketan alanwaketan marked this pull request as draft December 14, 2022 19:11
@alanwaketan
Copy link
Collaborator Author

This is still a WIP as I'm puzzled on the way how we trigger BuildInputOutputAliases and then I still need to add it to run_tests.sh. @JackCaoG Can you provide more insights?

@JackCaoG
Copy link
Collaborator

@alanwaketan do you still plan to merge this pr?

@alanwaketan
Copy link
Collaborator Author

@alanwaketan do you still plan to merge this pr?

Can we talk more about this tomorrow? I have some questions.

@JackCaoG
Copy link
Collaborator

Let me actually play with your code a bit, the way that BuildInputOutputAliases is written is confusing, I might need to add some comment to it or I will forget how it works very quickly.

@alanwaketan
Copy link
Collaborator Author

Let me actually play with your code a bit, the way that BuildInputOutputAliases is written is confusing, I might need to add some comment to it or I will forget how it works very quickly.

Sounds good. Feel free to develop on the branch.

@JackCaoG
Copy link
Collaborator

JackCaoG commented Dec 17, 2022

Ah I know what's going on, by

t2 = t2 + 2

you created an intermediate tensor with new tensor ID and assign it back to t2, if you do

t2 += 2

the alias count incremented as expected

@JackCaoG
Copy link
Collaborator

Let me rebase and add some comments to the code

@alanwaketan
Copy link
Collaborator Author

Ah I know what's going on, by

t2 = t2 + 2

you created an intermediate tensor with new tensor ID and assign it back to t2, if you do

t2 += 2

the alias count incremented as expected

This is tricky. Thanks for the investigation!

@JackCaoG JackCaoG force-pushed the alanwaketan/build_input_output branch from 04daadd to f6ea706 Compare December 17, 2022 01:30
Copy link
Collaborator

@JackCaoG JackCaoG 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 bit weird, but I am going to approve my own change lol

@alanwaketan alanwaketan marked this pull request as ready for review December 17, 2022 02:47
@JackCaoG JackCaoG force-pushed the alanwaketan/build_input_output branch from f6ea706 to b453444 Compare December 19, 2022 19:27
@alanwaketan
Copy link
Collaborator Author

I guess we can land this now, and later I can add more test cases for views. Thanks for helping this, Jack.

@JackCaoG
Copy link
Collaborator

@alanwaketan yea I will merge this once all test passed.

@JackCaoG JackCaoG force-pushed the alanwaketan/build_input_output branch from b453444 to c0f8d63 Compare December 19, 2022 19:41
@JackCaoG JackCaoG merged commit 27660d7 into master Dec 19, 2022
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.

3 participants