Skip to content
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

Issue with use of BINOMIAL_LOG_LIKELIHOOD loss #35

Closed
JoseAF opened this issue Jan 26, 2023 · 4 comments
Closed

Issue with use of BINOMIAL_LOG_LIKELIHOOD loss #35

JoseAF opened this issue Jan 26, 2023 · 4 comments

Comments

@JoseAF
Copy link

JoseAF commented Jan 26, 2023

Hi

I have moved from 0.2.3 to main (latest) and have now found an issue with my code:

[INFO 2023-01-26T12:20:50.093951+00:00 gradient_boosted_trees.cc:452] Default loss set to BINOMIAL_LOG_LIKELIHOOD
[FATAL 2023-01-26T12:20:50.0948034+00:00 YggdrasilWrapper.cpp:177] INVALID_ARGUMENT: No class registered with key "BINOMIAL_LOG_LIKELIHOOD" in the class pool "class yggdrasil_decision_forests::model::gradient_boosted_trees::AbstractLoss". Registered classes are "". Add as a dependency the cc_library rule that defines this class in your BUILD file

I can see that the loss functionality has changed quite a bit since 0.2.3 but I'm not sure what I'm missing to get this to work again.

Thanks!

@JoseAF
Copy link
Author

JoseAF commented Jan 27, 2023

Sorry for coming back to this again but I'm still having this issue.

So far I've tried adding "//yggdrasil_decision_forests/learner/gradient_boosted_trees/loss:loss_imp_binomial" to the cc_library_ydf at \yggdrasil-decision-forests\yggdrasil_decision_forests\learner\gradient_boosted_trees\BUILD and building the lib again but this doesn't fix the issue. Perhaps I should add it somewhere else?

I have also tried adding:

CreateLoss(proto::Loss::BINOMIAL_LOG_LIKELIHOOD, proto::Task::CLASSIFICATION, myFeatureCol, config)

to my code (although probably this is not needed, as I'm not using early stopping). This doesn't help either...

@achoum
Copy link
Collaborator

achoum commented Jan 27, 2023

Hi JoseAF,

Sorry to hear about this.

As you noted, the part of the code related to losses has changed. Losses are now using a registration mechanism similarly as for the learner, models and dataset formats. In most existing code, I would expect no changes in the user code. But I could imagine corner cases where you would have to register those losses manually (that is to link :loss_imp_binomial, or one of its dependents).

Following is the path used to inject :loss_imp_binomial in most user code. It could be useful to identify if you need to inject the loss manually:

The loss implementation "BINOMIAL_LOG_LIKELIHOOD" is defined in the file yggdrasil_decision_forests/learner/gradient_boosted_trees/loss/loss_imp_binary_focal.h and compiled in the rule :loss_imp_binomial of the file https://github.com/google/yggdrasil-decision-forests/blob/main/yggdrasil_decision_forests/learner/gradient_boosted_trees/loss/BUILD. Your error suggest that none of the losses are registered. For our tools, those losses are registered in the build rule :all_implementations of in file https://github.com/google/yggdrasil-decision-forests/blob/main/yggdrasil_decision_forests/learner/gradient_boosted_trees/loss/BUILD. ;all_implementations is used in yggdrasil_decision_forests/learner/gradient_boosted_trees, which is itself used in the build rule :all_learners or https://github.com/google/yggdrasil-decision-forests/blob/main/yggdrasil_decision_forests/learner/BUILD.

If this does not solve this issue, please try the bazel query tool. Notably, identify the path from your binary to :loss_imp_binomial. If such path does not exist, you have to register :loss_imp_binomial (or one of its decedents) manually.

@JoseAF
Copy link
Author

JoseAF commented Jan 30, 2023

Thanks for the reply Mathieu.

In my setting (as it comes from YDF checkout), BinomialLogLikelihoodLoss is defined in loss_imp_binomial.h and registered there as BINOMIAL_LOG_LIKELIHOOD. In gradient_boosted_trees/loss:BUILD we have the cc_library_ydf:loss_imp_binomial and also the cc_library_ydf:all_implementations which includes it. This all_implementations in turn is a dependency of cc_library_ydf:gradient_boosted_trees (at gradient_boosted_trees/BUILD file). When I get to the learner/BUILD file, I see that cc_library_ydf:all_learners depends on all_learners_except_hparam_optimizer, which in turn depends on gradient_boosted_trees. So there seems to be a defined path from learner to BINOMIAL_LOG_LIKELIHOOD - that's what made me think perhaps I'm missing something in the code that now needs to be added (e.g. CreateLoss...).

I'll have a look at the bazel query tool...

@JoseAF
Copy link
Author

JoseAF commented Jan 30, 2023

Hi guys

I haven't got this working yet. It seems all the dependencies are correct in the BUILD files as far as I can see. I have built a lib with tensorflow in case I was missing something from there but the error persists. I have also added a CreateLoss call and a set_loss call to try to trigger registering of the BINOMIAL_LOG_LIKELIHOOD loss but no luck. I'll keep trying, but any ideas are welcomed here...

Just for your information, in case it helps, the code I have works perfectly well with 0.2.3 and I haven't made changes to the BUILD files that I get from github.

Thanks

@achoum achoum closed this as completed Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants