Fix ordering of Python callbacks.#11812
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the ordering of Python callbacks by changing the internal storage from a set to an ordered list while maintaining deduplication. The fix ensures that callbacks are executed in the order they are provided to the training function.
- Changed callback storage from
settolist(dict.fromkeys())to preserve insertion order - Added comprehensive test to verify callback execution order is maintained
- Added
Optionalimport for type hints in the test
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python-package/xgboost/callback.py | Changed CallbackContainer.__init__ to use list(dict.fromkeys(callbacks)) instead of set(callbacks) to preserve callback ordering while still removing duplicates |
| tests/python/test_callback.py | Added Optional import and new test_preserve_order test that verifies callbacks are executed in the specified order using custom callback classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
cc @rongou |
| is_cv: bool = False, | ||
| ) -> None: | ||
| self.callbacks = set(callbacks) | ||
| self.callbacks = list(dict.fromkeys(callbacks)) |
There was a problem hiding this comment.
Curious, is dict.fromkeys() here just for removing duplicates? Why do we even allow duplicates in the callback list? IMO it should be better just throw an error when there are duplicates instead of silently removing them.
There was a problem hiding this comment.
Curious, is dict.fromkeys() here just for removing duplicates?
Yes, it's just to remove duplicates.
I agree. I don't recall the exact case, but when I rewrote the callback interface years ago, some other parameter tuning methods unnecessarily appended their callbacks to a list repeatedly.
I can try to raise an error in the next major release, this patch is supposed to go into a patch release for bug fix only.
There was a problem hiding this comment.
Thanks for explanation. When will this patch release be out?
There was a problem hiding this comment.
Let's continue the discussion in the original thread #11811 (comment) .
Close #11811