Skip to content

Conversation

@parsifal-47
Copy link
Contributor

Hi All,
This is a fix for #17483
the inconsistency caused by pad_width and pad_value being second argument, this PR moves pad_value to the attributes.
Let me know if that is not a proper way to resolve the issue, additionally I am not sure if there is a way to make a test for show, I would appreciate references in this case.

Thank you!

@yongwww
Copy link
Member

yongwww commented Dec 29, 2024

@tvm-bot rerun

3 similar comments
@yongwww
Copy link
Member

yongwww commented Dec 30, 2024

@tvm-bot rerun

@parsifal-47
Copy link
Contributor Author

@tvm-bot rerun

@Cookiee235
Copy link
Contributor

@tvm-bot rerun

@parsifal-47
Copy link
Contributor Author

@Cookiee235 I do not understand the workflow completely, but it looks like some approval is needed in order to run the rest of the PR checks

@yongwww
Copy link
Member

yongwww commented Jan 18, 2025

@tvm-bot rerun

@parsifal-47 parsifal-47 force-pushed the relax-inconsistent-print branch from 84d74e2 to 9d45cdb Compare January 26, 2025 03:24
@parsifal-47 parsifal-47 force-pushed the relax-inconsistent-print branch from fc4c7c2 to e38971a Compare January 26, 2025 23:30
@parsifal-47
Copy link
Contributor Author

parsifal-47 commented Jan 26, 2025

tests are working now, but when going from tensorflow in tests/python/contrib/test_msc/test_translate_tensorflow.py the constant is not getting folded and remains Var or getting converted to Var for some reason, not sure how to fix that.

I tried adding transforms to AttrCvt with fold_constant, but it looks like the Var appears somewhere after.

@parsifal-47
Copy link
Contributor Author

to elaborate a bit more, originally, when pad_value was passed as positional argument, it was folded automatically, but because pad_width was actually the second positional argument, printer was broken. After I moved it to attributes, automatic folding no longer applies and it looks like legalizer is not a good place to do that because I do not see any other foldings there. Looks like the best place is in conversion from tf because other tests are green, but I tired multiple combinations and haven't found any workable, will take another look in a few days, meanwhile if you have an advice, let me know, thank you!

@yongwww
Copy link
Member

yongwww commented Feb 17, 2025

Hi @parsifal-47 I opened another PR #17657, incorporated your effort, and co-authored there. The pad roundtripable should be fixed now. I will close this pr, feel free to chime in if you have any further comments

@yongwww yongwww closed this Feb 17, 2025
@parsifal-47
Copy link
Contributor Author

Hi @parsifal-47 I opened another PR #17657, incorporated your effort, and co-authored there. The pad roundtripable should be fixed now. I will close this pr, feel free to chime in if you have any further comments

sounds good, thank you!

@tqchen
Copy link
Member

tqchen commented Feb 17, 2025

thank you @parsifal-47 @yongwww

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