Closed caleb-kaiser closed 5 months ago
Attention: Patch coverage is 90.58824%
with 8 lines
in your changes missing coverage. Please review.
Project coverage is 40.84%. Comparing base (
907a51a
) to head (2739760
). Report is 142 commits behind head on main.:exclamation: Current head 2739760 differs from pull request most recent head 466a6b7
Please upload reports for the commit 466a6b7 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
optuna_integration/comet.py | 90.58% | 8 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
comet_ml
@nzw0301 no problem! I added Comet to the pyproject.toml
and fixed an issue with one of the tests. There's a linting problem with chainernm.py
, but since I didn't modify that file in my PR, I didn't want to overstep and change anything there. It seems like all other tests are passing though :)
@caleb-kaiser Thank you for the PR! We are checking if there are any issues regarding the license of comet-ml, so it may still take some time till we start the PR review!
P.S. I did not notice that you are one of the maintainers of comet-ml... So does this mean it is no problem for your side to place comet-ml integration in our repository?
This pull request has not seen any recent activity.
@nabenabe0928 Sorry for any confusion! I work for Comet ML, and I'm part of the engineering team that maintains comet-ml. It is no problem for us to place the integration in Optuna. It does not cause any issues relating to licensing. Is there anything specific about comet-ml or licensing I can answer for you that might help?
@caleb-kaiser Sorry for the late response. I was checking with our legal team and the request from our side is to replace the License stated in PyPI with an open-source license, preferably MIT license, to be on the safe side. Especially, the current license mentions that "this package can not be copied ..." and it can technically violate the condition, which may potentially lead to unwanted issues for us.
For this reason, I would like you to replace the license and then we can proceed to merge this PR.
Btw, what pip install does, which is to copy the package, is already violating the condition in the current license according to our legal team.
This pull request has not seen any recent activity.
This PR is not stale.
This pull request has not seen any recent activity.
This pull request was closed automatically because it had not seen any recent activity. If you want to discuss it, you can reopen it freely.
@nabenabe0928 Thank you for the feedback/your patience on this. We've just completed the process of re-licensing the Comet SDK to be MIT licensed! Does this address any concerns about integrating Optuna and Comet?
Thanks for the update and adding the license! Could you resolve the conflict and fix CIs?
This pull request has not seen any recent activity.
This pull request has not seen any recent activity.
This pull request has not seen any recent activity.
Ping @HideakiImamura @nabenabe0928
This pull request has not seen any recent activity.
@caleb-kaiser Could you fix the CI?
Hi @HideakiImamura, I'm a colleague from @caleb-kaiser. We got the fix ready internally but @caleb-kaiser didn't had the time to sent it before his vacation, sorry about that. He will send it early next week.
Hi @HideakiImamura ! The changes have been made, and it looks like everything is passing!
This pull request has not seen any recent activity.
I'm so sorry that there is a conflict in .github/workflows/checks.yml
. Could you resolve it? I will merge this PR after the conflict is resolved immediately.
@HideakiImamura it looks like all tests are passing now :)
This pull request has not seen any recent activity.
Motivation
I work on many of Comet ML's open source efforts, and recently, we've had a number of users request an official integration with Optuna. I implemented a straightforward integration between Comet and Optuna that provides a similar API to other experiment tracking platforms who've integrated with Optuna previously. Happy to incorporate any feedback the team might have! My assumption is that I should open a separate PR for documentation only after/if the team decides to approve the integration PR, but please let me know if that is not the case.
Description of the changes
CometCallback
class inoptuna_integration/comet.py
that can be passed in as a callback tostudy.optimize()
.@CometCallback.track_in_comet
decorator that allows users to decorate objective functions and directly access their Trial's Comet Experiment.tests/test_comet.py
, in keeping with this repository's testing guidelinesTesting
To see the integration in action, you can run the following code (note you'll need to have Comet installed and setup with a free API key, for which there are instructions here: https://www.comet.com/docs/v2/guides/getting-started/quickstart/ )
Attached are screenshots of the output of the above code, which you can also view here: https://www.comet.com/ckaiser/optuna-distributed-single-example
Optimizer report view:
Experiment dashboard view:
Single experiment view: