Skip to content
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

Draft: refactor(python): rename unit.io test dir to unit.read_write to prevent conflation with builtin io module #7109

Closed

Conversation

alexander-beedie
Copy link
Collaborator

@alexander-beedie alexander-beedie commented Feb 22, 2023

The recent #6961 inadvertently reverted the #6889 fix (as these test directories aren't really packages/modules, they shouldn't have an __init__, or it can cause the error in the linked PR, reproduced below).

Fatal Python error: Py_Initialize: can't initialize sys standard streams

So, we can do one of two things:

  1. Remove the __init__ file in the unit.io subdir again.
  2. Rename the unit.io directory so it can't conflict with the builtin module.

Since it's plausible that we may end up accidentally reintroducing __init__ files in the future, I've opted for no.2, renaming this tests subdir from io to read_write to prevent any unintended regressions later on.

@github-actions github-actions bot added python Related to Python Polars refactor Code improvements that do not change functionality labels Feb 22, 2023
@stinodego
Copy link
Member

stinodego commented Feb 22, 2023

We will need __init__.py files in some test folders, as otherwise test modules with the same name will clash. For example, you could imagine a test_explode.py under the namespaces folder and also under a dataframe folder. This will cause pytest to error if there are no init files. So I think it's better to be consistent and just use __init__.py files in every folder to make sure this issue doesn't pop up.

What are you doing that causes confusion with the builtin io package? Importing directly from a test module? I fail to see the case where this is a problem. Enlighten me 😄

EDIT: Nevermind, just reproduced the issue (should read better). Hmm...

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Feb 22, 2023

EDIT: Nevermind, just reproduced the issue (should read better). Hmm...

It's especially noticeable if you're debugging from PyCharm, as the right-click "debug" (at module level or individual test level) sets itself up in such a way as to trigger the fatal error every single time, so you can only debug from the console...

Screenshot 2023-02-22 at 23 44 29

I think it's better to be consistent and just use init.py files in every folder to make sure this issue doesn't pop up.

Keeping the __init__ files is harmless as long as they don't clash with a builtin module, so I've left them alone and just renamed the subdir 👍

@stinodego
Copy link
Member

stinodego commented Feb 22, 2023

Actually now I can't reproduce this (had different errors)... could you give me the step-by-step for triggering this error?

I would like our test structure to be able to mimic the main package structure, so I'm not a fan of this renaming. If I can reproduce the error, maybe I can find a different solution.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Feb 22, 2023

Actually now I can't reproduce this (had different errors)... could you give me the step-by-step for triggering this error?

Use PyCharm and try to debug the tests; you'll likely crash out with a fatal error :)
(In order to debug, I have to manually delete the __init__ to stop io being masked).

@stinodego
Copy link
Member

Is there any console command I can run to trigger this? Not a PyCharm user myself.

Not trying to be obnoxious here - but that will help me track down what the real issue is here.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Feb 22, 2023

Not trying to be obnoxious here - but that will help me track down what the real issue is here.

I suspect it comes down to where your relative working directory is, and the default test root associated with debugging in PyCharm is such that the io masks the builtin if it has an __init__ 💭 Might be reproducible by modifying the PYTHONPATH or working directory before launching python, and then running tests?

I don't currently have a CLI/console version for replication; I only tracked it down the first time because I suddenly started getting weird errors when trying to debug (and then again when it came back this week) :(

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Feb 22, 2023

Ah; you might have trouble replicating because apparently I accidentally committed the removal of io.__init__ in #7095; didn't mean to, but as I needed to debug a test I had deleted it, and that change got out into the wild... (this was going to be the follow-up PR after seeing that the __init__ had come back). If you put it back, might be easier to see the error.

@alexander-beedie alexander-beedie changed the title refactor(python): rename unit.io test dir to unit.read_write to prevent conflation with builtin io module Draft: refactor(python): rename unit.io test dir to unit.read_write to prevent conflation with builtin io module Feb 22, 2023
@stinodego
Copy link
Member

stinodego commented Feb 23, 2023

I still can't get it to error, even with the __init__.py in the py-polars/tests/unit/io folder.

Is this still an issue for you without an __init__.py in the io folder, like on the current master branch? Then I propose to just keep it like this for now.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Feb 23, 2023

Is this still an issue for you without an __init__.py in the io folder, like on the current master branch?

Nope; this completely fixes it as the directory no longer behaves like a module.

Then I propose to just keep it like this for now.

How about I add a simple unit test that checks if that __init__ exists, and have it fail with an error that explains why we don't currently want one there if it finds one? Then we can keep io as-is, and us PyCharm heretics will be safe from unintended future regressions :)

@alexander-beedie
Copy link
Collaborator Author

Closed in favour of much lighter PR: #7116.

@alexander-beedie alexander-beedie deleted the rename-io-testdir branch February 23, 2023 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to Python Polars refactor Code improvements that do not change functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants