[dynamo] Clean up assert in dynamo [1/N]

[can-gaa-hou]

Fixes some part of #162852 and #164878. These two issues have some relationship though.

• -> [dynamo] Clean up assert in dynamo [1/N] #165430

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela @williamwen42 @albanD

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/165430

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• [ROCm][CI] Machines under the label linux.rocm.gpu.2 are undergoing maintenance.

✅ You can merge normally! (1 Unrelated Failure)

As of commit dcdbab2 with merge base beb6b62 ():

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / before-test / llm-retrieval (gh) (matched llm-retrieval rule in flaky-rules.json)
  Process completed with exit code 2.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[albanD]

@anijain2305 can you confirm these are the right way to update these?

[Lucaskabela]

Some minor nits but generally this looks close to ready

For ease of review, can you also add an example of running a program that triggers one of these exceptions and the error before/after in the summary? That can serve as the testplan here - thanks!

[williamwen42]

meta point (cc @anijain2305)

The pattern

msg = ConstantVariable.create(...)
raise_observed_exception(TypeError, tx, args=[msg])

shows up a lot - could we refactor this?

[Lucaskabela]

Thanks for the changes - I will second @williamwen42's comment that we should put this in a helper fn since it appears so much. We can probably add it to variables/base.py. Something like

def raise_type_error_exc(tx: InstructionTranslator, msg_str: str) -> None:
    msg = ConstantVariable.create(msg_str)
    raise_observed_exception(TypeError, tx, args=[msg])

[Lucaskabela]

I think we dropped the check for len(args) here

[can-gaa-hou]

pytorch/torch/_dynamo/variables/lists.py

Lines 150 to 156 in 21697fe

	if len(args) != 1:
	msg = ConstantVariable.create(
	f"{name} takes exactly one argument ({len(args)} given)"
	)
	raise_observed_exception(TypeError, tx, args=[msg])
	assert not kwargs and len(args) == 1

Yeah.. But we can combine these two into one check because len(args) != 1 promises len(args) == 1, doesn't it?

[Lucaskabela]

nit: let's keep the order (kwargs or len(args) != 1) for consistency

[can-gaa-hou]

Fixed

[williamwen42]

@Lucaskabela I was also thinking of a followup where we potentially don't need to even hardcode the TypeError's string - this would probably involve somehow calling the function with the invalid arguments just to get the TypeError.

I wonder if inspect.signature.bind does this without actually calling the function? (update: inspect.signature.bind does not give the same TypeError message`)

[can-gaa-hou]

Thanks for the changes - I will second @williamwen42's comment that we should put this in a helper fn since it appears so much. We can probably add it to variables/base.py. Something like

def raise_type_error_exc(tx: InstructionTranslator, msg_str: str) -> None:
    msg = ConstantVariable.create(msg_str)
    raise_observed_exception(TypeError, tx, args=[msg])

@Lucaskabela Thanks for the advice. I have added this fn into variables/base.py and replaced some similar lines of code in this PR. I also notice that there are many other similar lines of code remaining. I would like to fix some here in case there are still some points we need to discuss. Once the pattern is set, I can clean up the rest.

inspect.signature.bind does not give the same TypeError message`

import inspect
def func(a, b):
    pass

sig = inspect.signature(func)
try:
    sig.bind(1, 2, 3)
except TypeError as e:
    print(e) # too many positional arguments

try:
    func(1, 2, 3)
except TypeError as e:
    print(e)  # func() takes 2 positional arguments but 3 were given

It seems like inspect.signature.bind gives less information than just calling it. Should we still consider it? @williamwen42

[can-gaa-hou]

Thanks for the changes - I will second @williamwen42's comment that we should put this in a helper fn since it appears so much. We can probably add it to variables/base.py. Something like

def raise_type_error_exc(tx: InstructionTranslator, msg_str: str) -> None:
    msg = ConstantVariable.create(msg_str)
    raise_observed_exception(TypeError, tx, args=[msg])

@Lucaskabela Thanks for the advice. I have added this fn into variables/base.py and replaced some similar lines of code in this PR. I also notice that there are many other similar lines of code remaining. I would like to fix some here in case there are still some points we need to discuss. Once the pattern is set, I can clean up the rest.

inspect.signature.bind does not give the same TypeError message`

import inspect
def func(a, b):
    pass

sig = inspect.signature(func)
try:
    sig.bind(1, 2, 3)
except TypeError as e:
    print(e) # too many positional arguments

try:
    func(1, 2, 3)
except TypeError as e:
    print(e)  # func() takes 2 positional arguments but 3 were given

It seems like inspect.signature.bind gives less information than just calling it. Should we still consider it? @williamwen42

[Lucaskabela]

LGTM - will probably need to rebase since another change to graph_break_registry landed

Might be easiest to:

1. Rebase on main
   2)git checkout main torch/_dynamo/graph_break_registry.json
2. Run lintrunner to repopulate the graph_break_registry change automagically

Thanks for working hard on this!

[can-gaa-hou]

LGTM - will probably need to rebase since another change to graph_break_registry landed

Might be easiest to:

1. Rebase on main
   2)git checkout main torch/_dynamo/graph_break_registry.json
2. Run lintrunner to repopulate the graph_break_registry change automagically

Thanks for working hard on this!

Still shows a conflict on GitHub o_O? Anyway, I rebased the branch and everything works fine now. Thanks for the review @Lucaskabela @williamwen42

[Lucaskabela]

Thanks for working hard to clean up asserts here! LGTM

[Lucaskabela]

Errr, seems that the gb registry isn't happy still - lintrunner should hopefully fix but if it still gives you trouble feel free to ping @williamwen42 who has more context on this

[williamwen42]

@can-gaa-hou the registry and the unimplemented_v2 fields are out of sync - just revert your graph_break_registry.json changes and apply lintrunner -a again

[williamwen42]

@pytorchbot merge

[pytorchmergebot]

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here