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

Pass --timeout flag to pyperf #354

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Pass --timeout flag to pyperf #354

merged 3 commits into from
Oct 2, 2024

Conversation

diegorusso
Copy link
Contributor

@diegorusso diegorusso commented Sep 19, 2024

Add the --timeout flag to the run command. By default there is no timeout and user needs to specify it if they want it. I wanted to avoid to change the current behaviour.
This addressed the issue #353

If you run the below command passing the timeout of 1 second, dask will fail and it will be reported at the end.

$ pyperformance run --debug-single-value -b float,go,mako,dask --timeout 1
...
...
Performance version: 1.11.0
Python version: 3.12.3 (64-bit)
Report on Linux-6.8.0-44-generic-aarch64-with-glibc2.39
Number of logical CPUs: 8
Start date: 2024-09-19 10:11:20.472768
End date: 2024-09-19 10:11:21.843698

### float ###
85.8 ms

### go ###
119 ms

### mako ###
9.77 ms

1 benchmarks failed:
- dask (Timed out after 1 seconds)

doc/usage.rst Outdated Show resolved Hide resolved
pyperformance/commands.py Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

pyperf main process does basically nothing, it just waits until worker processes complete. It may be safer to implement the timeout in pyperf, to kill both processes, the main one and the current worker process.

If you implement the timeout in pyperformance, you only see the main process and you cannot (easily) kill the worker process.

Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

I may have sent you forth with bad information by suggesting that this would be easier in pyperformance, and maybe @vstinner is right that this would be better handled in pyperf.

We'd still need a patch to pyperformance to pass the --timeout value down to pyperf -- 80% of this PR is handling commandline arguments as it is, so not wasted effort.

doc/usage.rst Outdated Show resolved Hide resolved
pyperformance/commands.py Show resolved Hide resolved
pyperformance/tests/__init__.py Outdated Show resolved Hide resolved
doc/usage.rst Outdated Show resolved Hide resolved
@diegorusso
Copy link
Contributor Author

I may have sent you forth with bad information by suggesting that this would be easier in pyperformance, and maybe @vstinner is right that this would be better handled in pyperf.

We'd still need a patch to pyperformance to pass the --timeout value down to pyperf -- 80% of this PR is handling commandline arguments as it is, so not wasted effort.

Don't worry, that's the purpose of the review. I leave this PR open for now and implement the timeout in pyperf. Once that one is in, I'll fix this up to make use of the timeout.

@diegorusso
Copy link
Contributor Author

I've addressed feedbacks on this PR and also raised a new one in pyperf: psf/pyperf#205

@diegorusso diegorusso changed the title Implement timeout mechanism for a benchmark run Pass --timeout flag to pyperf Sep 24, 2024
@diegorusso
Copy link
Contributor Author

Just to let you know that the pyperf PR has been merged (thanks @vstinner) so we have the timeout functionality there.

@vstinner
Copy link
Member

Just to let you know that the psf/pyperf#205 has been merged (thanks @vstinner) so we have the timeout functionality there.

You need a pyperf release first.

@mdboom
Copy link
Contributor

mdboom commented Sep 30, 2024

Just to let you know that the psf/pyperf#205 has been merged (thanks @vstinner) so we have the timeout functionality there.

You need a pyperf release first.

I started one here

@vstinner
Copy link
Member

vstinner commented Oct 1, 2024

Your PR should update pyperf to 2.8.

@diegorusso
Copy link
Contributor Author

Your PR should update pyperf to 2.8.

I see that pyperf is unpinned so in theory I should not change anything in there. I have done a fresh install of pyperformance and in fact it installs pyperf 2.8

@mdboom
Copy link
Contributor

mdboom commented Oct 1, 2024

Your PR should update pyperf to 2.8.

I see that pyperf is unpinned so in theory I should not change anything in there. I have done a fresh install of pyperformance and in fact it installs pyperf 2.8

I think you still need to update it here, which is the version that will actually run in the venvs.

@diegorusso
Copy link
Contributor Author

Your PR should update pyperf to 2.8.

I see that pyperf is unpinned so in theory I should not change anything in there. I have done a fresh install of pyperformance and in fact it installs pyperf 2.8

I think you still need to update it here, which is the version that will actually run in the venvs.

Yes, you are right. I've just found it! :)

@diegorusso
Copy link
Contributor Author

I'm currently running the tests because there is a small change in the way pyperf reports unit measure: psf/pyperf@432c419

Update test_commands to reflect a change how pyperf reports data.
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

Thanks!

@mdboom mdboom merged commit 22b2819 into python:main Oct 2, 2024
11 checks passed
@diegorusso diegorusso deleted the timeout branch October 2, 2024 14:14
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.

4 participants