TST / string dtype: add env variable to enable future_string and add test build #58459
Conversation
jorisvandenbossche
added
Testing
jorisvandenbossche
requested review from
phofl and
mroeschke
and removed request for
mroeschke
| @@ -858,7 +858,7 @@ def register_converter_cb(key: str) -> None: | |||
| with cf.config_prefix("future"): | |||
| cf.register_option( | |||
| "infer_string", | |||
| False, | |||
| True if os.environ.get("PANDAS_FUTURE_INFER_STRING", "0") == "1" else False, | |||
There was a problem hiding this comment.
can we make this something that is explicitly FOR_TESTING_ONLY or something to clarify users shouldn't use it
There was a problem hiding this comment.
I would personally prefer to keep the name a bit more readable. We did the same for CoW, I don't think we had any problems with it?
I also want to use this for testing in downstream packages, and would encourage others to do this as well (of course, that is still in a context of testing, but I feel that FOR_TESTING_ONLY would indicate it's only for testing for pandas devs)
(in general, there is not really a back compat issue with the env variable, in pandas 3.0 we will just start ignoring it, which is even easier than the explicit pd.options option, which will start raising an error at some point)
|
started a branch similar to this to get the tests passing; i count |
Impressive that a single test can give 810 failures ;) But then that set of failures should also be relatively easy to solve I suppose |
This comment was marked as outdated.
This comment was marked as outdated.
| # temporarily let pytest always succeed (many tests are not yet passing in the | ||
| # build enabling the future string dtype) | ||
| if [[ "$PANDAS_FUTURE_INFER_STRING" == "1" ]]; then | ||
| PYTEST_CMD="$PYTEST_CMD || true" | ||
| fi |
There was a problem hiding this comment.
I could also add a flag to pytest and then change the exit code from conftest.py (like in https://github.com/yashtodi94/pytest-custom_exit_code/blob/master/pytest_custom_exit_code.py), but since this is only temporary, this seems the easiest short term hack.
|
I am going to merge this as this makes testing further string dtype related PRs a lot easier. But happy to further bikeshed on the name and easy to change that later. |
xref #54792
Taking a similar approach as we did for CoW: allow enabling the option with an env variable (also useful for testing locally), and add an extra build on CI with that option enabled.
Not all tests are passing yet, so will see how to mark this build as "allow failures", but I think it would already be useful to set up the infra so we can fix the tests piecewise in follow-ups.