-
Notifications
You must be signed in to change notification settings - Fork 8
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
chore: 🤖 Apply BestieTemplate v0.7.2 #163
Conversation
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
I suggest dropping Cirrus CI @tmigot, what do you think? |
35bf913
to
7672df3
Compare
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
I personally agree. What do you think @amontoison ? |
Thanks, @amontoison. I still think we should delete Cirrus CI testing from this package. I don't think the main use case of this package requires testing on FreeBSD or a specific Mac that GH doesn't already provide. This is essentially an interface, so the underlying packages should be more thoroughly tested, but this doesn't need to. |
7672df3
to
76a12d0
Compare
The issue was that MacOS silicon was not supported by GHA until a very recent update. I think that Cirrus is relevant for packages that depend on Artifact (JLL packages) but we can remove it otherwise. |
76a12d0
to
4339f63
Compare
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Thanks! So, still good to me to remove cirrus for this package. |
Yes, we just need to update GHA to test on two architectures (x64 and aarch64). macos-13 is the last release of Mac that supports this architecture. macos-lastest is only working on aarch64 now. |
4339f63
to
24f782c
Compare
a262f90
to
1fb0288
Compare
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
I've updated the PR with the latest version of the template. I've removed CirrusCI. If we apply the template to other packages in JSO that use CirrusCI, we can update the template with the latest file. As for the extra tests, I also don't see the benefit of adding them now, in this package. The Lint failures are due to broken links. After merging, most or all should be fixed. |
Why? |
@dpo, ☝️ |
With that logic, there's no need to test on Windows or macOS either. I disagree. Do the FreeBSD tests not pass? The more platforms we cover, the more robust our code. |
I think the cost-benefit of adding FreeBSD is not good enough, but it is for Mac and Windows. That being said, if it makes it easier to adopt COPIERTemplate, I'm glad to reintroduce it. The file in the template is the following: https://github.com/abelsiqueira/COPIERTemplate.jl/blob/main/template/%7B%25%20if%20UseCirrusCI%20%25%7D.cirrus.yml%7B%25%20endif%20%25%7D.jinja My arguments for not having FreeBSD in this package are:
|
I just find it arbitrary to remove Cirrus from here while we use it everywhere else. I've found issues with code before because errors were triggered on a Cirrus VM. If we can cover another OS, why not? Predicting the effects of a change in one of the dependent packages is very hard, so the more coverage, the better, in my opinion. |
@dpo can you fit in your schedule maintaining the template's Cirrus file? |
Sure. there's really nothing to it. |
Great, I'll update this then |
1fb0288
to
6b3de17
Compare
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @abelsiqueira ! Sorry for the delay, but this is a massive PR^^. Are the modification to the code generated or some of them manually modified?
Feel free to let me know if you prefer that I open issues directly to COPIERTemplate
Please, before submitting, make sure that: | ||
|
||
- There is not an [existing issue](https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/issues) with the same question | ||
- You have read the [contributing guide](https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/blob/main/docs/src/90-contributing.md/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the link point to the documentation in the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update the template.
- You have read the [contributing guide](https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/blob/main/docs/src/90-contributing.md/) | ||
- You are following the [code of conduct](https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/blob/main/CODE_OF_CONDUCT.md) | ||
|
||
The form below should help you in filling out this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should more directly ask for a Minimal reproducible example. From what I've seen 75% of the first comment from a question is "Do you have a MWE?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update the template. There is a question for reproduction steps, I will add more info there.
- type: markdown | ||
attributes: | ||
value: | | ||
Thanks for taking the time to fill out this bug report! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to fill out this bug report! | |
Thanks for taking the time to fill out this feature request report! |
- You have read the [contributing guide](https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/blob/main/docs/src/90-contributing.md/) | ||
- You are following the [code of conduct](https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/blob/main/CODE_OF_CONDUCT.md) | ||
|
||
The form below should help you in filling out this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also suggest to explain ways to test the new request?
.github/PULL_REQUEST_TEMPLATE.md
Outdated
- [ ] Tests are passing | ||
- [ ] Lint workflow is passing | ||
- [ ] Docs were updated and workflow is passing | ||
<!-- - [ ] [CHANGELOG.md](https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/blob/main/CHANGELOG.md) was updated --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
[release-url]: https://github.com/JuliaSmoothOptimizers/JSOSuite.jl/releases | ||
|
||
One stop solutions for all things optimization. | ||
# JSOSuite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably worth adding somewhere adding that we use the COPIERTemplate in case somebody tries to update CI and stuff. what do you think?
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Applied the template and made some manual changes, such as changing the docs names, fixing the README and removing some older workflows. ✅ Closes: #143
93385a0
to
5ce990d
Compare
I've updated to BestieTemplate (it has been renamed) version 0.7.1. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
CirrusCI changed the rules recently and we only have a (very) limited number of credits now. |
If we're dropping it, it should be here so we can squash merge this, and because it is one of the question of the template and if we drop it we simplify the application more. |
@amontoison can you explain more on the changes or the limitation? @dpo had arguments to maintain, but if the limitation is too severe I imagine it might affect using Cirrus CI on other packages as well.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @abelsiqueira ! It looks good to me overall. I think it is a good place to have some experience on BestieTemplate.jl + it is quite easy to update with the template.
I would say just two things before merge:
- fix the DOI (see my comment)
- clarify cirrus
Thanks!!
Co-authored-by: Tangi Migot <[email protected]>
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
CirrusCI gives us 10,000 minutes (= 7 x 24 hours) per month for Linux VMs and 500 minutes for macOS VMs [1]. Is that such a limitation??? I'm a bit bummed by these discussions. The failure in this PR is unrelated to time limits. |
.cirrus.yml
Outdated
@@ -2,14 +2,14 @@ task: | |||
matrix: | |||
- name: FreeBSD | |||
freebsd_instance: | |||
image_family: freebsd-13-3 | |||
image_family: freebsd-13-2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
image_family: freebsd-13-2 | |
image_family: freebsd-13-3 |
.cirrus.yml
Outdated
env: | ||
matrix: | ||
- JULIA_VERSION: 1.6 | ||
- JULIA_VERSION: 1 | ||
- name: MacOS M1 | ||
macos_instance: | ||
image: ghcr.io/cirruslabs/macos-ventura-base:latest | ||
image: ghcr.io/cirruslabs/macos-monterey-base:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
image: ghcr.io/cirruslabs/macos-monterey-base:latest | |
image: ghcr.io/cirruslabs/macos-sonoma-base:latest |
Can you submit a RP to update https://github.com/abelsiqueira/BestieTemplate.jl/? Otherwise it defeats the purpose of having Cirrus it on the template. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/JSOSuite.jl/JSOSuite.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Applied the template and made some manual changes, such as changing the
docs names, fixing the README and removing some older workflows.
✅ Closes: #143