Repository navigation
TST: xfail part of test_partial_fit() on Linux - #138
tylerjereddy wants to merge 2 commits into
Conversation
* Related to lanlgh-117. That issue has been causing our CI to fail for almost 4 months now. While I was hoping someone would fix the matter properly, we can't continue to have CI failing all the time because we need to be able to assess the quality of other thrusts of work. * So, this branch proposes to `xfail` a subset of `test_partial_fit()` on Linux to avoid the failure. The issue will remain open--this is just to allow us to assess the correctness of incoming changes without interference from lanlgh-117. * I confirmed that this allows the full testsuite to pass on an x86_64 Linux machine with NumPy `2.5.4` installed from PyPI.
| (GFDLClassifier, False, "coeff_"), | ||
| (EnsembleGFDLClassifier, None, "coeffs_"), | ||
| pytest.param(EnsembleGFDLClassifier, None, "coeffs_", | ||
| marks=pytest.mark.xfail(condition="sys.platform == 'linux'", |
There was a problem hiding this comment.
Note that this is a bit more aggressive than strictly needed since I believe the issue only affects x86_64 Linux, and even then, only for a subset of the EnsembleGFDLClassifier cases/scenarios.
Still, this is reasonably simple, and shimming around only the single exact failure case would be a fair bit messier.
There was a problem hiding this comment.
CI is failing because the shim is also needed on Windows and because of connectivity issues similar to those reported in gh-98. I can push in a shim to help with the former, but the latter issue has been open for months without help and will need to be fixed separately.
There was a problem hiding this comment.
Ok, I've pushed in a commit to xfail test_partial_fit() similarly on windows for the ensembling flavor of the estimator. I used Opus 5.5 to add the Windows shim, since I can't test as easily on that platform, so I'll update my AI disclosure above shortly.
I did confirm manually that we still pass tests on Linux in the scenario above after that update.
* `xp.linalg.solve` documation for `partial_fit` in non-ensemble GFDL estimators * `softmax` from @activations.py achieves the same thing as in `partial_fit` custom written with `scipy.special.logsumexp` * These would eventually need to be addressed in an ensemble branch in the same way * Reverted to `rtol=1e-3` for `test_partial_fit` which had `pinv` instability in `EnsembleGFDLClassifier`. It looks like Tyler will `xfail` this case in lanl#138
* Adjust test_partial_fit() to also `xfail` on the Windows platform, because it is also affected by lanlgh-117.
* `xp.linalg.solve` documation for `partial_fit` in non-ensemble GFDL estimators * `softmax` from @activations.py achieves the same thing as in `partial_fit` custom written with `scipy.special.logsumexp` * These would eventually need to be addressed in an ensemble branch in the same way * Reverted to `rtol=1e-3` for `test_partial_fit` which had `pinv` instability in `EnsembleGFDLClassifier`. It looks like Tyler will `xfail` this case in lanl#138
|
@tylerjereddy : You already suggested a solution here and adopting that change makes all the partial fit tests pass for me on Chicoma (tried numpy versions 2.3.0, 2.4.0, 2.5.0-4). That chain of conversations seemed to agree that this would make our partial fit estimators robust. What is the blocker for adopting the solution you found ? |
* `xp.linalg.solve` documation for `partial_fit` in non-ensemble GFDL estimators * `softmax` from @activations.py achieves the same thing as in `partial_fit` custom written with `scipy.special.logsumexp` * These would eventually need to be addressed in an ensemble branch in the same way * Reverted to `rtol=1e-3` for `test_partial_fit` which had `pinv` instability in `EnsembleGFDLClassifier`. It looks like Tyler will `xfail` this case in lanl#138
@nray because the NumPy The numerical stability of the Do I currently trust that someone on the team can clean up the algorithms and testing confidently and cleanly? I'm not so sure, maybe, but not without a very large time investment and a likely difficult debate about how much we can relax our tests to make them more realistic. I'd welcome an attempt, and as I've noted in the issue, LLMs have interesting ideas, but at the cost of other current priorities? Seems like it could be a big timesink, and I've already invested a fair bit of time pushing on it. So, that's the main argument to punt for now. |
@nray That shim uses the keyword argument |
@eiviani-lanl and I have also identified challenges with
That may be the case. For tests that perform many combinations of all the RVFL argument values (1000s), we often/always end up with a few problematic inputs. |
One solution could be distinguish between which types of arrays we could allow to have a small proportion of mismatches. For instance, I would argue that the predicted values ought to be very (or uniformly) close, whereas matching all fitted coefficients could be unrealistic. IIRC, the linear algebra experts in the OpenBLAS discussions mentioned that matching all coefficients would be challenging.
This is less brittle when increasing model sizes versus 1 single mismatch out of say 10000. |
|
I scanned with For estimators where I found a model attribute being For estimators like For decompositions ( Overall, I don't see evidence of scikit-learn |
This above was a bad suggestion by me. I don't think that'll remedy the situation, at least not for Array API tests with float32. It may work for the ensemble estimator with float64. |
|
Just noting that I talked to @sdtemple and @nray separately in person this morning. There seemed to be some general agreement that we might relax some of our I believe @nray had a frontier model check I suggest carrying the discussion/analysis forward in that issue and coordinating there, and once a clear path of action is decided there you can just supersede my two PRs related to this matter if a new PR doing the "right thing" is opened. I don't think this has to be too complicated, just justify our level of stringency/checking based on how |
* `xp.linalg.solve` documation for `partial_fit` in non-ensemble GFDL estimators * `softmax` from @activations.py achieves the same thing as in `partial_fit` custom written with `scipy.special.logsumexp` * These would eventually need to be addressed in an ensemble branch in the same way * Reverted to `rtol=1e-3` for `test_partial_fit` which had `pinv` instability in `EnsembleGFDLClassifier`. It looks like Tyler will `xfail` this case in lanl#138
* `xp.linalg.solve` documation for `partial_fit` in non-ensemble GFDL estimators * `softmax` from @activations.py achieves the same thing as in `partial_fit` custom written with `scipy.special.logsumexp` * These would eventually need to be addressed in an ensemble branch in the same way * Reverted to `rtol=1e-3` for `test_partial_fit` which had `pinv` instability in `EnsembleGFDLClassifier`. It looks like Tyler will `xfail` this case in lanl#138
* `xp.linalg.solve` documation for `partial_fit` in non-ensemble GFDL estimators * `softmax` from @activations.py achieves the same thing as in `partial_fit` custom written with `scipy.special.logsumexp` * These would eventually need to be addressed in an ensemble branch in the same way * Reverted to `rtol=1e-3` for `test_partial_fit` which had `pinv` instability in `EnsembleGFDLClassifier`. It looks like Tyler will `xfail` this case in lanl#138
Related to TST, CI:
test_partial_fitis failing in Linux CI (with NumPy2.5.0) #117. That issue has been causing our CI to fail for almost 4 months now. While I was hoping someone would fix the matter properly, we can't continue to have CI failing all the time because we need to be able to assess the quality of other thrusts of work.So, this branch proposes to
xfaila subset oftest_partial_fit()on Linux to avoid the failure. The issue will remain open--this is just to allow us to assess the correctness of incoming changes without interference from TST, CI:test_partial_fitis failing in Linux CI (with NumPy2.5.0) #117.I confirmed that this allows the full testsuite to pass on an x86_64 Linux machine with NumPy
2.5.4installed from PyPI.AI usage disclosure: Claude Opus 5.5 was used to extend the
xfailto include the Windows platform after I manually handled the Linux part (accessing a windows machine is possible, but a bit annoying for me)