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

Add an __init__.py in repo root to prevent import confusion #949

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Jul 20, 2024

It makes me a little bit uneasy to put an __init__.py file outside of a legit Python package, but that's also sort of the point. I'm interested to know what people think.

Description

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@ricardoV94
Copy link
Member

So why are we special to need this? Should the inner pytensor directory be called src?

@maresb
Copy link
Contributor Author

maresb commented Jul 20, 2024

We are not at all special; this is a completely general Python annoyance.

I do prefer the src layout since it reduces confusion with the Python package names. However, it wouldn't have prevented #947.

Note: While I prefer src/ I'm also annoyed because src/ is a misnomer. It is actually "the root directory for Python packages", which frustratingly is quite a few too many words to turn into a sensible directory name. I have a slight preference for python/ which is more accurate but slightly less conventional than src/. All options are pretty bad IMHO.

@maresb
Copy link
Contributor Author

maresb commented Jul 20, 2024

Hmm, 362b1b3 seems to be a workaround for a pytest bug

@maresb
Copy link
Contributor Author

maresb commented Jul 20, 2024

Oh weird, it works for me locally

@maresb
Copy link
Contributor Author

maresb commented Jul 20, 2024

Oh, this is pretty funny...

I have pytest v8.1.1 locally, but when I upgrade to v8.2.2 I get the same behavior as the CI. That's annoying!

@maresb maresb marked this pull request as draft July 21, 2024 16:12
@maresb
Copy link
Contributor Author

maresb commented Jul 21, 2024

I'm not entirely serious about actually merging this one. The root of this problem is Python itself, so a workaround like this is likely to introduce more problems than it solves.

This is more of a proof-of-concept, to probe for misconfigurations, and to explore what nasty hacks are being used by mypy and pytest. Indeed as I'm discovering, pytest and mypy use some very nasty hacks.

@maresb
Copy link
Contributor Author

maresb commented Jul 21, 2024

Going a bit deeper, the PR that causes the root __init__.py to be imported is pytest-dev/pytest#12208. The reason is that this change causes parent modules to be loaded.

But why does the root __init__.py qualify as a parent module? It seems like a few things are going on:

This block is responsible for converting a path into a pair (pkg_root, module_name), e.g. ~/repos/pytensor/pytensor/tensor/math.py gets converted to ("~/repos/pytensor", "pytensor.tensor.math"). But pkg_root is determined by walking up directories as long as __init__.py exists. So with the extra __init__.py, things get wrongly split into ("~/repos", "pytensor.pytensor.tensor.math").

Additionally, it seems that having conftest.pyin the repo root causes __init__.py in the same directory to get imported. I'm actually pretty confused.

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.

BUG: Unable to install pytensor as a package with pip install -e
3 participants