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

Optimize notebook validation #2053

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sacpis
Copy link
Collaborator

@sacpis 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
@sacpis sacpis marked this pull request as draft August 6, 2024 22:07
Copy link

github-actions bot commented Aug 6, 2024

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Aug 6, 2024
Copy link

github-actions bot commented Aug 7, 2024

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Aug 7, 2024
@sacpis sacpis marked this pull request as ready for review August 7, 2024 18:58
@sacpis sacpis requested a review from bmhowe23 August 7, 2024 18:58
Copy link

github-actions bot commented Aug 9, 2024

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Aug 9, 2024
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Aug 11, 2024
@sacpis
Copy link
Collaborator Author

sacpis commented Aug 12, 2024

@bmhowe23 Would you be able to review this PR?

Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Aug 12, 2024
Comment on lines 32 to 36
if any(
re.search(pattern, lines) for pattern in
['set_target[\\\s\(]+"(.+)\\\\"[)]', '--target ([^ ]+)']):
matches = re.findall(
'set_target[\\\s\(]+"(.+)\\\\"[)]|--target ([^ ]+)', lines)
Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Collaborator Author

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).

Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants