Skip to content

[FX] Assert None concrete_args and improve error messages #74662

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.

Sign up for GitHub

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

Jump to bottom
Closed
jamesr66a wants to merge 4 commits into from
Closed

[FX] Assert None concrete_args and improve error messages #74662

jamesr66a wants to merge 4 commits into from

Conversation

@jamesr66a
Copy link
Collaborator

@jamesr66a jamesr66a commented Mar 24, 2022 •
edited
Loading

Stack from ghstack (oldest at bottom):

Previously, we would not emit a check that concrete_args with value None matched that value during runtime. This fixes that and improves some of the warning messages

Differential Revision: D35137362

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 24, 2022 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit b692bfd (more details on the Dr. CI page):

  • 3/3 failures introduced in this PR

🕵️ 2 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See GitHub Actions build pull / linux-docs / build-docs (cpp) (1/2)

Step: "Build cpp docs" (full log | diagnosis details | 🔁 rerun)

2022-03-25T19:41:15.5360981Z ImportError: canno...da/lib/python3.7/site-packages/jinja2/__init__.py) 
2022-03-25T19:41:15.5354637Z   File "/opt/conda/lib/python3.7/site-packages/sphinx/cmd/build.py", line 25, in <module>
2022-03-25T19:41:15.5355158Z     from sphinx.application import Sphinx
2022-03-25T19:41:15.5355838Z   File "/opt/conda/lib/python3.7/site-packages/sphinx/application.py", line 42, in <module>
2022-03-25T19:41:15.5356433Z     from sphinx.registry import SphinxComponentRegistry
2022-03-25T19:41:15.5357129Z   File "/opt/conda/lib/python3.7/site-packages/sphinx/registry.py", line 24, in <module>
2022-03-25T19:41:15.5357664Z     from sphinx.builders import Builder
2022-03-25T19:41:15.5358324Z   File "/opt/conda/lib/python3.7/site-packages/sphinx/builders/__init__.py", line 26, in <module>
2022-03-25T19:41:15.5358957Z     from sphinx.util import import_object, logging, rst, progress_message, status_iterator
2022-03-25T19:41:15.5359678Z   File "/opt/conda/lib/python3.7/site-packages/sphinx/util/rst.py", line 22, in <module>
2022-03-25T19:41:15.5360195Z     from jinja2 import environmentfilter
2022-03-25T19:41:15.5360981Z ImportError: cannot import name 'environmentfilter' from 'jinja2' (/opt/conda/lib/python3.7/site-packages/jinja2/__init__.py)
2022-03-25T19:41:15.5696672Z Makefile:27: recipe for target 'html' failed
2022-03-25T19:41:15.5696985Z make: *** [html] Error 1
2022-03-25T19:41:15.5740451Z ##[error]Process completed with exit code 2.
2022-03-25T19:41:15.5779427Z ##[group]Run pytorch/pytorch/.github/actions/chown-workspace@master
2022-03-25T19:41:15.5779679Z env:
2022-03-25T19:41:15.5779825Z   IN_CI: 1
2022-03-25T19:41:15.5779991Z   IS_GHA: 1
2022-03-25T19:41:15.5780165Z ##[endgroup]
2022-03-25T19:41:15.5793962Z ##[group]Run docker run --rm -v "$(pwd)":/v -w /v "${ALPINE_IMAGE}" chown -R "$(id -u):$(id -g)" .
2022-03-25T19:41:15.5794309Z �[36;1mdocker run --rm -v "$(pwd)":/v -w /v "${ALPINE_IMAGE}" chown -R "$(id -u):$(id -g)" .�[0m

See GitHub Actions build pull / linux-xenial-py3.7-gcc5.4 / test (backwards_compat, 1, 1, linux.2xlarge) (2/2)

Step: "Test" (full log | diagnosis details | 🔁 rerun)

2022-03-25T19:38:16.4466415Z The PR is introduc...m to confirm whether this change is wanted or not. 
2022-03-25T19:38:16.4454627Z processing existing schema:  text(__torch__.torch.classes.profiling.SourceRef _0) -> (str _0)
2022-03-25T19:38:16.4455950Z processing existing schema:  count(__torch__.torch.classes.profiling.InstructionStats _0) -> (int _0)
2022-03-25T19:38:16.4457016Z processing existing schema:  duration_ns(__torch__.torch.classes.profiling.InstructionStats _0) -> (int _0)
2022-03-25T19:38:16.4458409Z processing existing schema:  source(__torch__.torch.classes.profiling.SourceStats _0) -> (__torch__.torch.classes.profiling.SourceRef _0)
2022-03-25T19:38:16.4460282Z processing existing schema:  line_map(__torch__.torch.classes.profiling.SourceStats _0) -> (Dict(int, __torch__.torch.classes.profiling.InstructionStats) _0)
2022-03-25T19:38:16.4461053Z processing existing schema:  __init__(__torch__.torch.classes.profiling._ScriptProfile _0) -> (NoneType _0)
2022-03-25T19:38:16.4462553Z processing existing schema:  enable(__torch__.torch.classes.profiling._ScriptProfile _0) -> (NoneType _0)
2022-03-25T19:38:16.4463551Z processing existing schema:  disable(__torch__.torch.classes.profiling._ScriptProfile _0) -> (NoneType _0)
2022-03-25T19:38:16.4465204Z processing existing schema:  _dump_stats(__torch__.torch.classes.profiling._ScriptProfile _0) -> (__torch__.torch.classes.profiling.SourceStats[] _0)
2022-03-25T19:38:16.4466170Z processing existing schema:  __init__(__torch__.torch.classes.dist_rpc.WorkerInfo _0, str _1, int _2) -> (NoneType _0)
2022-03-25T19:38:16.4466415Z The PR is introducing backward incompatible changes to the operator library. Please contact PyTorch team to confirm whether this change is wanted or not. 
2022-03-25T19:38:16.4466423Z 
2022-03-25T19:38:16.4466526Z Broken ops: [
2022-03-25T19:38:16.4466740Z 	aten::_pad_circular(Tensor self, int[] pad) -> (Tensor)
2022-03-25T19:38:16.4466801Z ]
2022-03-25T19:38:16.5369849Z + cleanup
2022-03-25T19:38:16.5369953Z + retcode=1
2022-03-25T19:38:16.5370030Z + set +x
2022-03-25T19:38:16.5408935Z ##[error]Process completed with exit code 1.
2022-03-25T19:38:16.5459793Z ##[group]Run pytorch/pytorch/.github/actions/chown-workspace@master
2022-03-25T19:38:16.5459866Z env:

1 failure not recognized by patterns:

Job Step Action
GitHub Actions pull / linux-docs / build-docs (python) Unknown 🔁 rerun
This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

jamesr66a pushed a commit that referenced this pull request Mar 24, 2022
[FX] Assert None concrete_args and improve error messages
6dc5cd6 
ghstack-source-id: d94e15e
Pull Request resolved: #74662
@jamesr66a jamesr66a requested review from Chillee, ansley, suo and zdevito and removed request for Chillee March 24, 2022 04:05
Update on "[FX] Assert None concrete_args and improve error messages"
4430e78 
Previously, we would not emit a check that `concrete_args` with value `None` matched that value during runtime. This fixes that and improves some of the warning messages


[ghstack-poisoned]
Update on "[FX] Assert None concrete_args and improve error messages"
8d8c1fd 
Previously, we would not emit a check that `concrete_args` with value `None` matched that value during runtime. This fixes that and improves some of the warning messages


[ghstack-poisoned]
jamesr66a pushed a commit that referenced this pull request Mar 25, 2022
[FX] Assert None concrete_args and improve error messages
8ee1a18 
ghstack-source-id: 8705847
Pull Request resolved: #74662
@jamesr66a jamesr66a mentioned this pull request Mar 25, 2022
Closed
@jamesr66a
Copy link
Collaborator Author

jamesr66a commented Mar 25, 2022

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

Chillee
Chillee approved these changes Mar 25, 2022
View reviewed changes
Copy link
Collaborator

@Chillee Chillee left a comment

Choose a reason for hiding this comment

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

Makes sense for what it does.

I do kinda wonder whether these checks are a good idea in the first place though... I'm worried it just adds unnecessary complexity/don't always work.

Although, I'm guessing that you want these guards here since you find them helpful, in which case, ignore that :P

@jamesr66a
Copy link
Collaborator Author

jamesr66a commented Mar 25, 2022

@Chillee yeah I was helping @pbelevich debug an issue in PiPPy that probably would have been more clear if we had these guards

Update on "[FX] Assert None concrete_args and improve error messages"
b692bfd 
Previously, we would not emit a check that `concrete_args` with value `None` matched that value during runtime. This fixes that and improves some of the warning messages

Differential Revision: [D35137362](https://our.internmc.facebook.com/intern/diff/D35137362)

[ghstack-poisoned]
facebook-github-bot pushed a commit that referenced this pull request Mar 25, 2022
@facebook-github-bot
[FX] Assert None concrete_args and improve error messages (#74662)
b960405 
Summary:
Pull Request resolved: #74662

Previously, we would not emit a check that `concrete_args` with value `None` matched that value during runtime. This fixes that and improves some of the warning messages

Test Plan: Imported from OSS

Reviewed By: Chillee

Differential Revision: D35137362

Pulled By: jamesr66a

fbshipit-source-id: 222a2c8a907748f90290f1c1b4ab8012b46099a0
@facebook-github-bot facebook-github-bot deleted the gh/jamesr66a/435/head branch March 29, 2022 14:17
@WBobby WBobby mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Chillee Chillee Chillee approved these changes

@zdevito zdevito Awaiting requested review from zdevito

@suo suo Awaiting requested review from suo

@ansley ansley Awaiting requested review from ansley

Assignees

No one assigned

Labels

cla signed module: fx

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jamesr66a @facebook-github-bot @Chillee