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 "how it works" and "caveats" sections to readme #24

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

ngoldbaum
Copy link
Contributor

I think adding the code example makes it a bit more explicitly clear what exactly the plugin is doing.

@rgommers rgommers added the documentation Improvements or additions to documentation label Jan 15, 2025
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @ngoldbaum, good idea! A few minor comments, and CI isn't happy yet.

README.rst Outdated
Given an existing test, this plugin creates a new test that is equivalent to
the following:

```python
Copy link
Member

Choose a reason for hiding this comment

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

CI is unhappy because this is a .rst file, not .md.

README.rst Outdated


with ThreadPoolExecutor(max_workers=num_parallel_threads) as tpe:
b = threading.Barrer(num_parallel_threads)
Copy link
Member

Choose a reason for hiding this comment

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

typo here, Barrier

README.rst Outdated
@@ -49,6 +48,31 @@ Features
* ``num_iterations``: The number of iterations the test will run in each
thread

Explanation
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd call this slightly differently (maybe "How it works"), and move it above Features

Copy link
Member

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

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

Good improvement. I left a couple of comments.

README.rst Outdated
Comment on lines 62 to 63
b.wait()
for _ in range(num_iterations):
Copy link
Member

@lysnikolaou lysnikolaou Jan 15, 2025

Choose a reason for hiding this comment

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

Waiting for the barrier happens in every iteration.

Suggested change
b.wait()
for _ in range(num_iterations):
for _ in range(num_iterations):
b.wait()

README.rst Outdated
```

Using this plugin avoids the boilerplate of rewriting existing tests to run in
parallel in a thread pool.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note about trying our best to do proper error propagation as well?

@ngoldbaum
Copy link
Contributor Author

I also added a caveats section to help explain some of the limitations from #14.

README.rst Outdated
import threading
from concurrent.futures import ThreadPoolExecutor

def run_test(b):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe declare *args and **kwargs to explicitly indicate that fixtures are shared across concurrent test instances?

@ngoldbaum ngoldbaum changed the title Add explanation to readme Add "how it works" and "caveats" sections to readme Jan 15, 2025
@andfoy
Copy link
Member

andfoy commented Jan 16, 2025

@ngoldbaum, is this one ready?

@ngoldbaum
Copy link
Contributor Author

I think so! OK to merge?

@andfoy andfoy merged commit 8c5f136 into Quansight-Labs:main Jan 17, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants