Use package-relative imports to avoid collisions with user code

[snarfed]

Hi @MarshalX et al! First off, thanks for building and maintaining this library. It's great!

When I first tried it just now, I immediately hit an ImportError. I have an atproto_firehose.py module in my project's root, and atproto doesn't use package-relative imports, so it tried to import its own atproto_firehose, got mine instead, and choked:

ImportError: Failed to import test module: test_atproto_firehose
Traceback (most recent call last):
  File "/opt/homebrew/Cellar/[email protected]/3.11.9/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/loader.py", line 162, in loadTestsFromName
    module = __import__(module_name)
             ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ryan/src/bridgy-fed/tests/test_atproto_firehose.py", line 28, in <module>
    import atproto_firehose
  File "/Users/ryan/src/bridgy-fed/atproto_firehose.py", line 27, in <module>
    from atproto import CAR, AtUri, FirehoseSubscribeReposClient, firehose_models, models, parse_subscribe_repos_message
  File "/Users/ryan/src/bridgy-fed/local/lib/python3.11/site-packages/atproto/__init__.py", line 11, in <module>
    from atproto_firehose import (
ImportError: cannot import name 'AsyncFirehoseSubscribeLabelsClient' from partially initialized module 'atproto_firehose' (most likely due to a circular import) (/Users/ryan/src/bridgy-fed/atproto_firehose.py)

You probably want to make all of your internal imports package-relative by adding a . to prevent this. Eg in this case, you'd do from .atproto_firehose import ...

[MarshalX]

Hi! Great to see you here. My pleasure!

You should not name file/module as another module. Because it will hide original module by local one. In your case please rename atproto_firehose.py to something else.

Let me know will it help or not

[MarshalX]

I mean I understand the problem but using a relative path is not the right approach here. IMHO.

atproto package is just a single file project which only provides shortcuts to other packages. Probably it covers 80% of import needs for developers but not 100%.

I mean, for example, if you want to use something deeper or cover all the code with type hints you may need to import additional things. And you will be forced to import some package directly. For example atproto_firehose one. And the problem described in this issue will hit you again.

[snarfed]

Hey, good to see you too, thanks for the reply!

Users will have all sorts of modules and filenames of their own, and they install all sorts of different packages from pip. You can't know ahead of time which files your users will have, and they can't know ahead of time which files you have. It's very unusual, and not really reasonable, to require users to know and avoid all filenames that any of their dependencies use, either at the beginning or on an ongoing basis as everyone adds new files over time.

Fortunately, package-relative imports solve this by giving every package its own namespaces! No more filename collisions.

I know it will probably be an annoying chore to update all of your imports, but package-relative imports are generally expected in the ecosystem, and definitely worthwhile so that your users don't hit problems like these.

[snarfed]

I mean, for example, if you want to use something deeper or cover all the code with type hints you may need to import additional things. And you will be forced to import some package directly. For example atproto_firehose one. And the problem described in this issue will hit you again.

Sure, but I can import atproto_firehose package-relative and alias it, eg from atproto import atproto_firehose as atproto_sdk_firehose.

If all of your internal imports are global, your package immediately breaks when I first try to use it, and I'm forced to rename files in my project to fix that breakage, which is unnecessary and a bad experience.

[MarshalX]

Once again, package-related imports will save you only when you will perform “import atproto”. And even in this case, you are not allowed to create “atproto.py”

This is so typical and annoying Python mistake. I personally sometimes create, for example, “json.py” and can't use a built-in json parser XD

[MarshalX]

Oh we have desync in replies

[snarfed]

Right! A big part of the difficulty here is that you put a bunch of atproto_* packages into the top level namespace, which is unusual. Usually those would all be subpackages of a single top-level atproto package, which would avoid this problem. The code I'm writing now only needs to import identifiers from atproto anyway, not directly from any of the atproto_* packages.

People expect that if they install the atproto package, they can't also have their own atproto.py file. Built in modules like json are the same, you're right! But it will definitely surprise some people that the atproto package also adds a bunch of other top-level atproto_* packages that have a decent chance of colliding with their own code. I've been doing open source Python for 15+ years, and it surprised me. 😁

(As background, out of curiosity, I looked at a handful of popular open source Python libs - requests, click, attrs, flask, cryptography, dateutil, babel, bases, docutils, dns - and they all use package-local imports, either relative with from . import ... or absolute with eg from requests import ...)

[snarfed]

I definitely get that this won't be easy to change! Especially if your users are currently importing from these top-level atproto_* packages. Moving them to be subpackages will be a breaking change, which will take time and effort.

[MarshalX]

Yeah, this is my first project where I changed the structure to multi-top-level packages. The idea was to make it completely indented and maybe even publish it on PyPI separately. So the user can cherry-pick only the necessary part of the SDK. Like we have in the NPM ecosystem. But they have namespaces for the packages... and I am not sure that the current state of SDK is fully separated into independent packages. Firehose depends on the client, the client depends on core, etc... but you can probably install just the client without firehose (in the world where SDK is published to many PyPI packages).

That's why all imports are not relative. Yeah, right now we have only one PyPI package. So I need to think about the future of it and do we need any action here

Thank you for raising this!

[MarshalX]

And yeah because of missed namespaces in the Python ecosystem, all packages have the same prefix “atproto_”. But unfortunately it could easily collide with users' codebase :(

[snarfed]

Understood! Yeah that's always a tension with a big project. Breaking it up into separate packages seems like a good idea for managing the complexity...but that moves some of the complexity onto your users to handle, ie understanding which of the many separate packages do they need for what they're trying to do.

Still worthwhile sometimes! But complicated.