[python-package][PySpark] Expose Training and Validation Metrics #11133
Conversation
ayoub317
force-pushed
the
expose-metrics
branch
9 times, most recently
from
3d7161a to
99fc349
Compare
ayoub317
force-pushed
the
expose-metrics
branch
from
99fc349 to
25a06b8
Compare
|
|
||
|
|
||
| @dataclass | ||
| class _XGBoostTrainingSummary: |
There was a problem hiding this comment.
Could be XGBoostTrainingSummary ?
| @@ -0,0 +1,43 @@ | |||
| """Xgboost training summary integration submodule.""" | |||
There was a problem hiding this comment.
how about renaming xgboost_training_summary.py to summary.py?
ayoub317
force-pushed
the
expose-metrics
branch
from
a1f9df2 to
469c6bb
Compare
|
Hi @ayoub317, Could you be able to add some unit tests for this feature? |
|
Hi @wbo4958, Yes, with pleasure ! Could you please provide some pointers on where I should define these tests ? I had a quick look at the test codebase, and I would assume the following: The tests for the regressor and classifier could be placed under: The test for the ranker could be placed under: Does that sound like a good choice ? Thanks ! |
|
The path you pasted should be ok for adding new testing. |
ayoub317
force-pushed
the
expose-metrics
branch
from
469c6bb to
ef56736
Compare
…gSummary to XGBoostTrainingSummary
ayoub317
force-pushed
the
expose-metrics
branch
8 times, most recently
from
5828a0c to
da80def
Compare
|
Thanks @wbo4958 ! I pushed another commit adding the tests. Any feedback is welcome. |
ayoub317
force-pushed
the
expose-metrics
branch
from
da80def to
3e60eec
Compare
| @@ -1148,7 +1151,7 @@ def _train_booster( | |||
| if dvalid is not None: | |||
| dval = [(dtrain, "training"), (dvalid, "validation")] | |||
| else: | |||
| dval = None | |||
| dval = [(dtrain, "training")] | |||
There was a problem hiding this comment.
@trivialfis, Could you check this is ok by enabling it by default?
|
|
||
| from .test_spark_local import spark as spark_local | ||
|
|
||
| logging.getLogger("py4j").setLevel(logging.INFO) |
There was a problem hiding this comment.
Yes, and since it was also set in test_spark_local.py, I kept it. Do you prefer that we remove it ?
| @@ -0,0 +1,233 @@ | |||
| import logging | |||
There was a problem hiding this comment.
I'm wondering, if we could put the tests in this file into the existing test_spark_local.py and reuse the existing test data?
There was a problem hiding this comment.
Yes, I can move them there without much effort. Let me know if you'd like me to proceed with that.
However, in my humble opinion, it’s better to keep them in this separate file, and here’s the rationale :
The test_spark_local.py file already exceeds 1800 lines of code, which makes it increasingly difficult to read, maintain and navigate. As new features are added to PySpark XGBoost, this file will only continue to grow, compounding the problem.
I think refactoring the tests to organize them by key features, rather than bundling everything under the TestPySparkLocal class would be a better long-term approach.
If we decide to keep the tests in this file, I can either leave the examples here as they are, or, as you suggested, for better modularity and data reuse, we could import them from test_spark_local.py. Another option is to store all shared data in a separate file, allowing both test_spark_local.py and test_xgboost_summary.py to import what they need from it.
Let me know what you think, I have no strong opinion on this.
There was a problem hiding this comment.
Yeah, That's good point. Originally, I would like to separate the tests per the estimators. like XGBoostClassifier/Regressor/Ranker, instead of per features. So you can share the same dataset for different features.
| assert not xgb_model.training_summary.validation_objective_history | ||
|
|
||
| @staticmethod | ||
| def assert_non_empty_training_objective_history( |
There was a problem hiding this comment.
I'm wondering if we could get the evaluate_results from xgboost itself and the training summary from xgboost-pyspark on the same dataset, and then check if they are equal? You can see some tests in test_spark_local.py are doing same comparison.
There was a problem hiding this comment.
Yes, absolutely. Thank you for pointing this out ! I tested this on a simple DataFrame locally, and the results matched perfectly. We should definitely add such tests, I’ll take care of that !
Closes #11132