Skip to content
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.

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

Add --timing flag, phase timing to @dynamo_timed #92637

Closed
wants to merge 10 commits into from
Closed

Conversation

voznesenskym
Copy link
Contributor

@voznesenskym voznesenskym commented Jan 19, 2023

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 19, 2023

🔗 Helpful Links

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

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

❌ 1 Failures

As of commit cd1b009:

NEW FAILURES - The following jobs have failed:

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

for key, time in timings.items():
total += time
print(f" {key} : {time}")
print(f"Total time: {total}s")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually sus cause of nesting

benchmarks/dynamo/final.csv Outdated Show resolved Hide resolved
torch/_dynamo/utils.py Outdated Show resolved Hide resolved
torch/_dynamo/utils.py Outdated Show resolved Hide resolved
torch/_dynamo/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

seems fine I guess

@albanD albanD removed their request for review January 19, 2023 23:47
@voznesenskym voznesenskym added the topic: not user facing topic category label Jan 19, 2023
@voznesenskym
Copy link
Contributor Author

@pytorchbot merge -f "local lintrunner -a doesn't show a diff. lintrunner init run, still no dice. I give up. yolo"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@malfet
Copy link
Contributor

malfet commented Jan 20, 2023

@pytorchbot revert -f "Broke lint" -c ignoredsignal

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 20, 2023

❌ 🤖 pytorchbot command failed:

@pytorchbot revert: error: the following arguments are required: -m/--message

usage: @pytorchbot revert -m MESSAGE -c
                          {nosignal,ignoredsignal,landrace,weird,ghfirst}

Try @pytorchbot --help for more info.

@malfet
Copy link
Contributor

malfet commented Jan 20, 2023

@pytorchbot revert -m "Broke lint" -c ignoredsignal

@malfet
Copy link
Contributor

malfet commented Jan 20, 2023

@pytorchbot merge -f "local lintrunner -a doesn't show a diff. lintrunner init run, still no dice. I give up. yolo"

Not sure what was the point of doing this? If commit breaks CI it will be reverted, it's just the matter of time...

@albanD
Copy link
Collaborator

albanD commented Jan 21, 2023

btw some lints (like flake8) cannot be automatically fixed. So it is expected that -a will not fix everything in some cases!

@voznesenskym
Copy link
Contributor Author

btw some lints (like flake8) cannot be automatically fixed. So it is expected that -a will not fix everything in some cases!

I was plagued with two problems:

  1. lintrunner -a does not look at everything (need lintrunner -m master)
  2. Unrelated failures - In this case, the failures are on lines I did not touch. @malfet advised that it is a bad base. (current state)

@voznesenskym
Copy link
Contributor Author

@pytorchmergebot rebase

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

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

@voznesenskym
Copy link
Contributor Author

@pytorchbot merge -f "Linter failure for code I did not touch"

image

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@kit1980
Copy link
Member

kit1980 commented Jan 22, 2023

"Linter failure for code I did not touch"

Looks like the MyPy failure is because the the decorator dynamo_timed was removed from _convert_frame_assert, changing its type.

@kit1980
Copy link
Member

kit1980 commented Jan 22, 2023

@pytorchbot revert -m "Broke lint again" -c ignoredsignal

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

Reverting PR 92637 failed

Reason: Command git -C /home/runner/work/pytorch/pytorch revert --no-edit 5778c04a15ebf2bdf9a309a70d1018588ac68b63 returned non-zero exit code 1

Auto-merging benchmarks/dynamo/common.py
Auto-merging benchmarks/dynamo/parse_logs.py
CONFLICT (content): Merge conflict in benchmarks/dynamo/parse_logs.py
error: could not revert 5778c04a15... Add `--timing` flag, phase timing to @dynamo_timed (#92637)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git revert --continue".
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".
Details for Dev Infra team Raised by workflow job

kit1980 added a commit that referenced this pull request Jan 22, 2023
#92637 broke lint, can't easily revert because of merge conflicts.
pytorchmergebot pushed a commit that referenced this pull request Jan 22, 2023
#92637 broke lint, can't easily revert because of merge conflicts.

Pull Request resolved: #92759
Approved by: https://github.com/ezyang
@voznesenskym
Copy link
Contributor Author

Oof sorry about this, I've been banging into the linter like an idiot all week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants