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

[Dynamo] Fix lineinfo generation on PY3.11+ #103525

Closed
wants to merge 6 commits into from

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Jun 13, 2023

  • Replace for inst in instructions[0:targe.offset//2]: inst.starts_line = None, with the one that that iterates over all instructions until inst.offset == target.offset condition is met, this way making it uniform across Python bytecode dialects (Python-3.11+ bytecode size is variable, while bytecode size is fixed for older Pythons)
  • Speedup target_index search by replacing [i for i in instructions if i.offset == offset][0] with next(i for i in instructions if i.offset == offset), which aborts the evaluation after condition met for the first time, according to:
     In [1]: lst=list(range(10000))
    
     In [2]: %time [i for i in lst if i == 10]
     CPU times: user 144 µs, sys: 23 µs, total: 167 µs
     Wall time: 168 µs
     Out[2]: [10]
    
     In [3]: %time next(i for i in lst if i == 10)
     CPU times: user 6 µs, sys: 0 ns, total: 6 µs
     Wall time: 9.06 µs
     Out[3]: 10
  • Fix small typo
  • use is_py311_plus variable rather than checking sys.version_info

🤖 Generated by Copilot at 6cd7f27

We fix the typos in our code of doom
We remove the warnings that obscure our vision
We refactor the generate function for the dynamo
We resume the execution with precision

Fixes #103355

cc @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @msaroufim

Do binary search for instruction index when on 3.11 rather than divide
index by 2

Fixes #103355
@pytorch-bot
Copy link

pytorch-bot bot commented Jun 13, 2023

🔗 Helpful Links

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

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

✅ No Failures

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

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

Copy link
Member

@williamwen42 williamwen42 left a comment

Choose a reason for hiding this comment

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

We could replace sys.version_info >= (3, 11) checks elsewhere as well.

@malfet
Copy link
Contributor Author

malfet commented Jun 13, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jun 13, 2023
@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@malfet
Copy link
Contributor Author

malfet commented Jun 13, 2023

Heh, I need to do linear search, as insn.offset can be None (i.e. offsets are not ascending)

@malfet malfet requested a review from williamwen42 June 13, 2023 23:37
@malfet
Copy link
Contributor Author

malfet commented Jun 14, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

@malfet malfet deleted the malfet/dynamo-py311-linetable branch August 2, 2023 02:39
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.

[dynamo] Lineinfo is wrong for Python-3.11
4 participants