Add Trackio Integration for Optuna #259
Conversation
|
@ParagEkbote Thank you for your pull request. Could you fix the CI error? I think you can skip Python 3.9 test by specifying |
c-bata
left a comment
There was a problem hiding this comment.
Thank you the updates! I left an early feedback comment.
Due to the year-end and New Year holidays, my review may take a bit longer than usual. I’ll get back to you once I’m able to resume the review.
Co-authored-by: c-bata <contact@c-bata.link>
|
This pull request has not seen any recent activity. |
|
Could you please review the changes? cc: @c-bata |
c-bata
left a comment
There was a problem hiding this comment.
I left two early feedback comments 🙏
tests/trackio/test_trackio.py
Outdated
| @@ -0,0 +1,203 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
My first impression is that most of these test scenarios rely heavily on unittest.mock, which makes it difficult to see the long-term value of maintaining them.
For example, if TrackioCallback were to break due to future changes in trackio, these tests would not detect that issue. Given this, I wonder if it might make sense to remove these tests. What do you think?
There was a problem hiding this comment.
I have modeled them similarly to the WeightsAndBiasesCallback tests, since the public api of trackio is modeled based on wandb. What type of alternative testing strategy do you think would work?
cc: @c-bata
There was a problem hiding this comment.
One option might be to remove these tests altogether. Since they rely heavily on mocks, I’m not sure they provide much value in catching real regressions in trackio itself.
The only alternative I can think of would be integration tests against the actual trackio behavior, but those tend to be fragile and hard to maintain. From a maintainability perspective, I would personally prefer to avoid such tests and instead address issues as they are reported.
There was a problem hiding this comment.
I have removed the tests and can also remove the gh-action additions if required . Could you please review?
Co-authored-by: c-bata <contact@c-bata.link>
There was a problem hiding this comment.
Thank you for the update. Changes itself look good to me. I will merge this PR after following CI issue will be resolved:
https://github.com/optuna/optuna-integration/actions/runs/20967917853/job/60551934863?pr=259
|
@ParagEkbote Thank you for your investigation. It seems that the cause of the problem is not related to your changes. I’m now investigating the root cause. 🙏 |
|
@ParagEkbote I fixed the CI issue at #264. Could you merge the latest main branch? 🙏 |
Fixes #248
Motivation
As described in the issue, this PR implements an
TrackioCallbackfor optuna-integration. Currently, trackio has a min python version of 3.10 and since python 3.9 has reached EOL, we will be required to bump the min. version in this repo to ensure that CI will be green.Description of the changes
The implementation of the callback is similar to the wandb callback, as the library is designed to be an open-source and offline alternative to wandb. The main difference between the wandb and trackio is that trackio does not rely on a global mutable state, each run is explicitly created and each run has a fixed lifetime. The API docs have also helped to define the scope of the callback.
The runs can be visualized as follows:
We can also display the results as an HF Space and an dataset. The example is as follows:
If the results are not synced, you can do so with the following command:
Could you please review?
cc: @nzw0301