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

ENH: Cleanup with asv_runner #1287

Merged
merged 12 commits into from
Jul 6, 2023
Merged

Conversation

HaoZeke
Copy link
Member

@HaoZeke HaoZeke commented May 7, 2023

Proof of concept implementation of #1286. What's left is:

  • [ ] Add a configuration option to select additional benchmarks mem
  • Add pympler in a backwards compatible manner

Adding additional benchmarks should be handled elsewhere, maybe in #1283, since for BC reasons, mem and pympler shouldn't have to be selected by users.

Closes #1286.

EDIT: Updated the name of the core repository

@HaoZeke
Copy link
Member Author

HaoZeke commented May 7, 2023

At the moment, if pympler is present; e.g. via:

    "matrix": {
        "req": {
            "pympler": []
        },
    },

then Mem benchmarks will be run, otherwise they're skipped silently.

@HaoZeke
Copy link
Member Author

HaoZeke commented May 13, 2023

At the moment, if pympler is present; e.g. via:

    "matrix": {
        "req": {
            "pympler": []
        },
    },

then Mem benchmarks will be run, otherwise they're skipped silently.

This is a backwards incompatible change. Since asv hardcodes the Mem benchmarks, downstream projects expect it to continue working, including the names of the benchmark results files and the requirements matrix. I guess the most compatible way is to ensure pympler is part of _base_requirements whenever possible (i.e., not on pypy).

@HaoZeke
Copy link
Member Author

HaoZeke commented May 14, 2023

@mattip this is feature complete, and ready for review, I'm rather baffled by the Appveyor failure, hoping I can deprecate appveyor altogether with #1119.

@HaoZeke HaoZeke requested a review from mattip May 14, 2023 23:02
pyproject.toml Outdated
@@ -25,6 +25,7 @@ dynamic = [
"version",
]
dependencies = [
"asvcore",
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the asvcore package on PyPI created? Is it from this repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently it's over here:

https://github.com/HaoZeke/asvcore

If the PR and implementation looks good the next step would be to migrate it to the airspeed-velocity organization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a design document somewhere with how these pieces fit together: what belongs in asvcore and what remains in asv? I am not sure I understand the division.

Copy link
Member Author

@HaoZeke HaoZeke May 15, 2023

Choose a reason for hiding this comment

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

The design and separation is described a bit in #1286

@mattip
Copy link
Contributor

mattip commented May 15, 2023

Where is the pyproject.toml for asvcore? It seems the PyPI entry is missing metadata like the repo, where to file issues, ...

@HaoZeke
Copy link
Member Author

HaoZeke commented May 15, 2023

Where is the pyproject.toml for asvcore?

For now:
https://github.com/HaoZeke/asv_runner

It seems the PyPI entry is missing metadata like the repo, where to file issues, ...

Yup I thought it would be better to fill those out once it's migrated to the airspeed-velocity organization instead of where it is now (a repo under my github handle).

assert len(b) == 36
if util.ON_PYPY:
assert len(b) == 34
else:
assert len(b) == 36
Copy link
Member Author

Choose a reason for hiding this comment

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

@mattip and I discussed that perhaps later it might make sense to have a mechanism for discovering all benchmarks, and then skipping or mark as fail or something where it isn't feasible to run.

Currently this design is because the regex is part of the classes and in the new approach the benchmark types are dynamically populated.

@HaoZeke
Copy link
Member Author

HaoZeke commented May 17, 2023

The appveyor failure might stem from an incorrect (hardcoded) file path separator as suggested by @mattip

@HaoZeke HaoZeke changed the title ENH: Cleanup with asvcore ENH: Cleanup with asv_runner Jun 5, 2023
@HaoZeke HaoZeke force-pushed the splitBench branch 6 times, most recently from 93e6ebc to 010588a Compare June 5, 2023 03:57
@HaoZeke
Copy link
Member Author

HaoZeke commented Jun 11, 2023

Incredibly the appveyor failure seems to stem from windows blacklisting aux.* as a filename.. will update asv_runner.

@HaoZeke
Copy link
Member Author

HaoZeke commented Jun 11, 2023

@mattip can we go ahead and merge this since the test failures have been fixed? The rest of the issues (documentation, repo location) can be worked on after that.

pyproject.toml Show resolved Hide resolved
@mattip
Copy link
Contributor

mattip commented Jun 12, 2023

This is a big change. Does it have some echo in the documentation? Maybe here?

@HaoZeke
Copy link
Member Author

HaoZeke commented Jun 25, 2023

@mattip I've added some documentation, and also started working on API documentation for asv_runner.

Let me know if it looks alright / what specific questions are not addressed :)

@HaoZeke HaoZeke requested a review from mattip June 25, 2023 00:24
@mattip mattip merged commit 5ab2d29 into airspeed-velocity:master Jul 6, 2023
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.

RFC: Factoring out asv.benchmark
2 participants