search

Ogeon / palette

A Rust library for linear color calculations and conversion
Apache License 2.0
749 stars 60 forks source link

chore(bench): update CodSpeed/action to v2 #391

Closed adriencaccia closed 4 months ago

adriencaccia commented 4 months ago

Upgrading to the v2 of https://github.com/CodSpeedHQ/action will bring a better base run selection algorithm, better logging, and continued support.

Ogeon commented 4 months ago

Hey, thanks for this! I hadn't noticed there was a v2. I'll let it run through everything to see that it's all good.

Edit: Oh, right, looks like I didn't include itself in its filter. :sweat_smile:

Ogeon commented 4 months ago

I will take care of that later. Thanks for this PR and all the work you put into this!

Ogeon commented 4 months ago

It looks like it's having trouble finding the benchmarks now. Is this a familiar issue? https://github.com/Ogeon/palette/actions/runs/8859407386/job/24329330697

adriencaccia commented 4 months ago

It looks like it's having trouble finding the benchmarks now. Is this a familiar issue? Ogeon/palette/actions/runs/8859407386/job/24329330697

This is because in the latest version of cargo-codspeed, benchmarks are now run in their package root directory. So here in the case of palette, they are not run in the integration_tests folder anymore, but in the benchmarks folder.

adriencaccia commented 4 months ago

@Ogeon if the path in https://github.com/Ogeon/palette/blob/ffcab42be71d9250581a7ef09edcf3ca354509d5/integration_tests/tests/convert/data_color_mine.rs#L329 is relative to the folder actually running the benchmarks (ie: benchmarks in our case), cargo codspeed run works again Maybe the solution would be to change a bit the load_data() function to ensure that it works, no matter what the current_dir() is?

Ogeon commented 4 months ago

Yeah, this current setup is definitely weird and should be changed, honestly. It's from when I had to split it into multiple crates so the benchmarks and integration tests can be independent of the minimum supported rust version.

I was using the working directory parameter so it could still find the CSV files, due to the relative paths, but has its purpose changed? Running the same commands locally seemed to work like before, so I'm curious to know how that differs here. What I tried was to build the benchmarks from the workspace root and then run them from integration_tests. It found everything as usual. 🤔

adriencaccia commented 4 months ago

I was using the working directory parameter so it could still find the CSV files, due to the relative paths, but has its purpose changed? Running the same commands locally seemed to work like before, so I'm curious to know how that differs here. What I tried was to build the benchmarks from the workspace root and then run them from integration_tests. It found everything as usual. 🤔

Have you udpated your local version of cargo-codspeed to the latest version, 2.6.0? It should not work with it. And since the CI takes the latest version, it is failing as well

Ogeon commented 4 months ago

I thought I did, but I'll have to check again. That would explain things. 🤔 It's about time I sort these files out anyway. I'll see if I give it another try on Wednesday, and get back to you about how it went. It's almost midnight where I live and tomorrow is a bit too busy.

adriencaccia commented 4 months ago

I found a way to make it work again in #398 😇 This is not a long term fix, but it has the merit of making everything work again.

Ogeon commented 4 months ago

Well, I can't complain when the whole thing already was a hack. 😁 Thanks a lot! I hope you don't mind that I wait with the merge until tomorrow, when I have the rest of my brain up and running again. It looks like everything should be all good, though!