pytensor icon indicating copy to clipboard operation
pytensor copied to clipboard

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

Open maresb opened this issue 1 year ago • 7 comments

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

  • [X] Closes #947

Checklist

Type of change

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

maresb avatar Jul 20 '24 09:07 maresb

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

ricardoV94 avatar Jul 20 '24 15:07 ricardoV94

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 avatar Jul 20 '24 15:07 maresb

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

maresb avatar Jul 20 '24 15:07 maresb

Oh weird, it works for me locally

maresb avatar Jul 20 '24 15:07 maresb

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 avatar Jul 20 '24 18:07 maresb

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 avatar Jul 21 '24 16:07 maresb

Going a bit deeper, the PR that causes the root __init__.py to be imported is https://github.com/pytest-dev/pytest/pull/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.

maresb avatar Jul 21 '24 21:07 maresb