jit trace will fail for parameter check if it contains param whose ki…

[sywangyi]

…nd is _ParameterKind.VAR_KEYWORD

[pytorch-bot]

🔗 Helpful Links

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

• 📄 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 or our office hours

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

✅ No Failures

As of commit ef4d483:
💚 Looks good so far! There are no failures yet. 💚

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: sywangyi / name: Wang, Yi (aa202b9)

[sywangyi]

the jit failure for bloom model whose forward contains “**deprecated_arguments” see https://github.com/huggingface/transformers/blob/main/src/transformers/models/bloom/modeling_bloom.py#L880

the return of [] of this function cause failure in check https://github.com/pytorch/pytorch/blob/master/torch/jit/_trace.py#L1036

[sywangyi]

@yao-matrix @jgong5 please help review

[davidberard98]

@sywangyi do you think you could add a test for the behavior that previously would fail? you can add it in test/jit/test_tracer.py

[sywangyi]

@sywangyi do you think you could add a test for the behavior that previously would fail? you can add it in test/jit/test_tracer.py

Hi. @davidberard98 thanks for the review. I have added it.

[facebook-github-bot]

@davidberard98 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[davidberard98]

@sywangyi can you take a closer look at the test failures? I think this change looks okay but please fix the test failures first.

also, I recommend you rebase/merge off of the viable/strict branch. This branch tracks the most recent commit on master that passes all CI tests.

[davidberard98]

adding my thoughts on why this change is reasonable, in case I forget later.

First, why I initially thought this might be concerning: we silently allow users to proceed in cases where the function takes **kwargs, which we don't support. It may not be obvious to the user why the failure is occuring.

Counterpoint (why this change is okay): This PR doesn't change the fact that **kwargs is silent; it just increases support for that scenario. In addition, this problem only occurs with example_kwarg_inputs (where you provide kwargs input to jit.trace, instead of a list of args when you use the usual example_inputs) so this improves the user experience.

[facebook-github-bot]

@davidberard98 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[davidberard98]

actually I retract my approval based on the test failures... can you take a look at those?

[sywangyi]

actually I retract my approval based on the test failures... can you take a look at those?

Hi. @davidberard98 . I check the failure case, one is test_coalesce_reference_cycle_cpu_float64 in test_sparse.py. I run all the test_sparse.py case in my env. all success. and even it does not enter the logic I change, I place a breakpoint there.
same as the failure case under "linux-bionic-py3.11-clang9"

[davidberard98]

@pytorchbot rebase -s

[davidberard98]

test_coalesce_reference_cycle_cpu_float64 looks unrelated, it is failing in a number of PRs. I am more concerned about test_empty_class_serialization, it sounds vaguely related.

Let's see if a rebase fixes the issues.

[pytorchmergebot]

@pytorchbot successfully started a rebase job. Check the current status here

[pytorchmergebot]

Successfully rebased bloom_trace onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout bloom_trace && git pull --rebase)

[facebook-github-bot]

@davidberard98 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[davidberard98]

@pytorchbot rebase -s

[pytorchmergebot]

@pytorchbot successfully started a rebase job. Check the current status here

[pytorchmergebot]

Successfully rebased bloom_trace onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout bloom_trace && git pull --rebase)

[facebook-github-bot]

@davidberard98 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[davidberard98]

@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