Skip to content

Fix xgboost errors in CI #1295

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

Merged
merged 1 commit into from
Mar 21, 2025
Merged

Fix xgboost errors in CI #1295

merged 1 commit into from
Mar 21, 2025

Conversation

cswartzvi
Copy link
Contributor

@cswartzvi cswartzvi commented Mar 21, 2025

This is an attempt to fix xgboost errors in CI related to test_xgboost_booster_json_writer and test_xgboost_booster_json_reader where the following error is encountered:

Check failed: base_score > 0.0f && base_score < 1.0f: base_score must be in (0,1) for logistic loss, got: 0

Currently blocking #1287 and #1294

Changes

I added a default base_score to the fitted_xgboost_booster fixture.

How I tested this

N/A

Notes

Oddly enough this, these errors never show up on my machine (Windows). Which is usually reversed!

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to f613993 in 1 minute and 4 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. tests/plugins/test_xgboost_extensions.py:21
  • Draft comment:
    Good fix ensuring base_score is in (0,1) for logistic loss. Consider adding a comment here or updating the docs to note this explicit setting is required due to xgboost changes.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    This is a test file, not the main implementation. The comment suggests documenting a XGBoost parameter constraint, but this constraint would be better documented in XGBoost's own docs. The change appears to be fixing a test by adding a required parameter, but explaining why in comments/docs isn't necessary for test code. Test code should demonstrate correct usage without needing extensive documentation.
    The constraint on base_score could be important for users of the hamilton XGBoost plugin to know. Not documenting it could lead to runtime errors.
    While the constraint is important, it's a XGBoost requirement, not specific to hamilton's implementation. The proper place for this documentation is in XGBoost's docs, not hamilton's test files.
    Delete the comment. Documentation about XGBoost parameter constraints belongs in XGBoost's documentation, not in hamilton's test files.
2. tests/plugins/test_xgboost_extensions.py:21
  • Draft comment:
    Set 'base_score' to 0.5 to fix CI errors. Consider adding an inline comment explaining why this parameter is explicitly set (e.g., due to cross-platform differences) and update docs if needed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    This is a test file, and the parameter change appears to be a simple fix for CI stability. The comment asks for documentation but doesn't provide evidence that the cross-platform explanation is actually correct. Adding inline comments in test files about implementation details like this is often unnecessary noise. The base_score parameter is a standard XGBoost parameter.
    I might be undervaluing the importance of documenting CI-related fixes. Future maintainers might need to understand why this specific value was chosen.
    While documentation can be valuable, test fixtures should be simple and self-evident. If this parameter is truly critical for cross-platform compatibility, it should be documented in the main codebase or CI configuration, not as an inline comment in a test fixture.
    The comment should be deleted as it requests unnecessary documentation in a test fixture and makes claims about cross-platform behavior without clear evidence.
3. tests/plugins/test_xgboost_extensions.py:37
  • Draft comment:
    Typo in the reason message of the xfail marker: 'scikitlearn' should be corrected to 'scikit-learn'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_DpZbC3DAh2TZwqIv


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@elijahbenizzy elijahbenizzy self-requested a review March 21, 2025 03:59
@elijahbenizzy
Copy link
Contributor

Thank you!

@elijahbenizzy elijahbenizzy merged commit 25422f4 into apache:main Mar 21, 2025
24 checks passed
@cswartzvi cswartzvi deleted the fix_xgboost branch March 21, 2025 13:11
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

Successfully merging this pull request may close these issues.

2 participants