-
Notifications
You must be signed in to change notification settings - Fork 185
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
Optimize notebook validation #2053
base: main
Are you sure you want to change the base?
Conversation
sacpis
commented
Aug 6, 2024
- Read the file content at once
- Encapsulate jupyter installation check in a function
- Uses a single regex search for all patterns
* Encapsulate jupyter installation check in a function * Uses a single regex search for all patterns
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
@bmhowe23 Would you be able to review this PR? |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
docs/notebook_validation.py
Outdated
if any( | ||
re.search(pattern, lines) for pattern in | ||
['set_target[\\\s\(]+"(.+)\\\\"[)]', '--target ([^ ]+)']): | ||
matches = re.findall( | ||
'set_target[\\\s\(]+"(.+)\\\\"[)]|--target ([^ ]+)', lines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble figuring out what this code change is doing. It looks more confusing to me with the duplicate strings in there. What exactly is this optimizing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble figuring out what this code change is doing. It looks more confusing to me with the duplicate strings in there. What exactly is this optimizing?
And I'm also curious which notebooks (if any) show a measurable difference in runtime because of this optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bmhowe23 for reviewing this change. I will put these patterns in a list instead.
This change avoids unnecessary computations by first ensuring that any pattern from the pattern list exists and if It it does not then it avoids further checks (if condition for a match).
If any pattern exists, then it uses re.findall
to collect all matching substrings in a single operation.
Basically, the idea for this PR is to read the entire file at once (f.read()). This avoids reading the file using f.readlines() (as it creates a list of string (per line) in memory, which could use high memory).
* creating a common list for patterns
…pis/cuda-quantum into optimize_notebook_validation
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |