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

remove dependency matrix from asv.ci.conf.json #16296

Merged
merged 5 commits into from Apr 18, 2024

Conversation

zacharyburnett
Copy link
Contributor

Description

tests changes to benchmarking environment from astropy/astropy-benchmarks#113

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

Copy link

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@zacharyburnett zacharyburnett marked this pull request as draft April 16, 2024 17:00
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@pllim

This comment was marked as resolved.

@pllim
Copy link
Member

pllim commented Apr 17, 2024

Interesting... 🤔 😸

| Change   | Before [c638d70a]    | After [0a37090e]    |   Ratio | Benchmark (Parameter)                                         |
|----------|----------------------|---------------------|---------|---------------------------------------------------------------|
| -        | 306±9μs              | 228±0.4μs           |    0.75 | units.TimeQuantityOpLargeArraySameUnit.time_quantity_np_equal |
| -        | 462±10μs             | 339±10μs            |    0.73 | units.TimeQuantityOpLargeArraySameUnit.time_quantity_np_add   |

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

@pllim
Copy link
Member

pllim commented Apr 17, 2024

How do I know just by inspecting the log (without the UI) what versions of what were used in the benchmarks here? I cannot figure it out looking at https://github.com/astropy/astropy/actions/runs/8722480045/job/23928443193?pr=16296

@zacharyburnett
Copy link
Contributor Author

How do I know just by inspecting the log (without the UI) what versions of what were used in the benchmarks here? I cannot figure it out looking at https://github.com/astropy/astropy/actions/runs/8722480045/job/23928443193?pr=16296

I'm not sure that is mentioned in the log; you would have to check the results files

@pllim
Copy link
Member

pllim commented Apr 17, 2024

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Apr 17, 2024

Hmm I see artifact at https://github.com/astropy/astropy/actions/runs/8722480045/artifacts/1422151899 from https://github.com/astropy/astropy/actions/runs/8722480045/job/23928443193?pr=16296 but it does not mention dependency versions.

ah I see now, the results artifact doesn't actually include the results; I reran with it uploading: https://github.com/astropy/astropy/actions/runs/8725413079/job/23938322175

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Apr 17, 2024

the results will only show dependency versions if specified in the matrix.req; we can include matplotlib and scipy with specific versions to matrix.req if it is necessary to freeze versions they are run against.

@pllim
Copy link
Member

pllim commented Apr 17, 2024

Thanks! But I still cannot find it. Maybe searching for "numpy" isn't enough? Are you able to?

https://github.com/astropy/astropy/actions/runs/8725413079/artifacts/1422883208

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Apr 18, 2024

here: https://github.com/astropy/astropy/actions/runs/8738886254

you can see the explicit numpy and matplotlib versions in these results because they're defined in the matrix.reqs; those will also show in the HTML results as a selectable matrix

@pllim
Copy link
Member

pllim commented Apr 18, 2024

I was thinking more like the pytest header we have or at least a pip freeze somewhere in the log. Is that possible?

Anyway, I think that is getting out of topic. The conclusion from this PR is that your astropy/astropy-benchmarks#113 is acceptable but only if we also update the JSON here ASAP afterwards.

Thanks!

@zacharyburnett
Copy link
Contributor Author

I tried adding pip freeze to the install_command; however, it did not show in the benchmark log at all. There might be a way to insert that into the log somehow in asv.conf.json

@pllim pllim modified the milestones: v7.0.0, v6.1.1 Apr 18, 2024
@pllim pllim added the backport-v6.1.x on-merge: backport to v6.1.x label Apr 18, 2024
@zacharyburnett zacharyburnett changed the title test changes to benchmark environment remove dependency matrix from asv.ci.conf.json Apr 18, 2024
@pllim pllim marked this pull request as ready for review April 18, 2024 18:50
@pllim
Copy link
Member

pllim commented Apr 18, 2024

RTD failure is unrelated, so I am gonna squash merge. Thanks!

@pllim pllim merged commit 802dc27 into astropy:main Apr 18, 2024
25 of 27 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Apr 18, 2024
@zacharyburnett zacharyburnett deleted the test/benchmark_environment branch April 18, 2024 19:24
pllim added a commit that referenced this pull request Apr 18, 2024
…296-on-v6.1.x

Backport PR #16296 on branch v6.1.x (remove dependency matrix from `asv.ci.conf.json`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v6.1.x on-merge: backport to v6.1.x benchmark Run benchmarks for a PR no-changelog-entry-needed testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants