search

coiled / benchmarks

BSD 3-Clause "New" or "Revised" License
28 stars 17 forks source link

migrate AB tests to smth compatible with depandabot #1437

Closed fjetter closed 6 months ago

fjetter commented 6 months ago

It happens rather frequently that I am running on rather old versions of dask/distributed in AB_environments. We're not doing a good job of updating those environments. I wanted to setup depandabot for this job but noticed that conda is not even supported.

I suggest to move off of conda for the A/B tests to smth else, e.g. pip / pip-compile that supports dependabot and let it update the dependencies at a regular cadence

crusaderky commented 6 months ago

Apologies for not noticing this ticket sooner.

I think that letting A/B tests diverge from nightly ones (ci/environment*.yml) would be a very bad idea. If we want to use dependabot, we should use it everywhere so that they remain in sync.

Also, the idea behind dependabot is that it will trigger CI for you to prove that everything keeps working. However, A/B tests do not run on a commit by default, so you may end up accepting changes without knowing if they cause a failure or not. Again this would be fixed by including the nightly environments in dependabot.

We also need to ensure that dependabot doesn't introduce performance regressions - so we should enable the regressions workflow for dependabot PRs only (it's currently only active for nightly runs).

Finally, I suspect that dependabot would trigger a lot of extra CI runs, and it would severely add to noise if it upgrades too frequently. I think we should investigate a setting to limit it to a bulk upload e.g. biweekly, instead of letting it create a new PR every time any one dependency does a minor release.

crusaderky commented 6 months ago

Alternatively, we should consider using dependabot for dask, dask-expr and distributed only. That would preempt a lot of my concerns above (nighlty runs are already running git tip anyway).

It happens rather frequently that I am running on rather old versions of dask/distributed in AB_environments.

If the problem is that you're accidentally forgetting to update dask/distributed/dask-expr version in the conda environment, wouldn't just changing those 3 packages not to have a pin (while all third-party dependencies remain pinned) solve the issue?

fjetter commented 6 months ago

I agree that we should also include the ordinary environments to depandabot.

However, I'm not particularly worried about regressions that are being introduced by depandabot. This makes regressions triaging a little more work but I also think this is happening rarely. If it's easy to enable this workflow, I'm happy to add it but I don't want us to spend much time on this.

Finally, I suspect that dependabot would trigger a lot of extra CI runs, and it would severely add to noise if it upgrades too frequently. I think we should investigate a setting to limit it to a bulk upload e.g. biweekly, instead of letting it create a new PR every time any one dependency does a minor release.

dependabot is running on an interval. See https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file schedule.interval. This is actually a required field

If the problem is that you're accidentally forgetting to update dask/distributed/dask-expr version in the conda environment, wouldn't just changing those 3 packages not to have a pin

not having a pin at all is not a good solution since I don't want things to wobble while a run is active.