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

python dataclasses seems to look in sys.modules[__module__], so all dynamic loading patterns would need to add there too (which...how do you clean up ...shrugs) #1419

Closed
kasium opened this issue Feb 9, 2024 · 11 comments
Labels
bug Something isn't working execution model wontfix This will not be worked on
Milestone

Comments

@kasium
Copy link
Contributor

kasium commented Feb 9, 2024

Describe the bug
If from __future__ import annotations is used with a dataclass, alembic crashes

Expected behavior
No crash

To Reproduce

  1. create a migration file as below
  2. run alembic heads
from __future__ import annotations

from dataclasses import dataclass

revision = "be59ab2abe34"
down_revision = "2677466a6831"

@dataclass
class Foo:
    name: str

def upgrade() -> None:
    pass


def downgrade() -> None:
    pass

Error

  File "venv/lib/python3.12/site-packages/alembic/config.py", line 624, in main
    self.run_cmd(cfg, options)
  File "venv/lib/python3.12/site-packages/alembic/config.py", line 601, in run_cmd
    fn(
  File "venv/lib/python3.12/site-packages/alembic/command.py", line 556, in heads
    heads = script.get_revisions(script.get_heads())
                                 ^^^^^^^^^^^^^^^^^^
  File "venv/lib/python3.12/site-packages/alembic/script/base.py", line 407, in get_heads
    return list(self.revision_map.heads)
                ^^^^^^^^^^^^^^^^^^^^^^^
  File "venv/lib/python3.12/site-packages/sqlalchemy/util/langhelpers.py", line 1113, in __get__
    obj.__dict__[self.__name__] = result = self.fget(obj)
                                           ^^^^^^^^^^^^^^
  File "venv/lib/python3.12/site-packages/alembic/script/revision.py", line 143, in heads
    self._revision_map
  File "venv/lib/python3.12/site-packages/sqlalchemy/util/langhelpers.py", line 1113, in __get__
    obj.__dict__[self.__name__] = result = self.fget(obj)
                                           ^^^^^^^^^^^^^^
  File "venv/lib/python3.12/site-packages/alembic/script/revision.py", line 197, in _revision_map
    for revision in self._generator():
  File "venv/lib/python3.12/site-packages/alembic/script/base.py", line 149, in _load_revisions
    script = Script._from_filename(self, dir_name, filename)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "venv/lib/python3.12/site-packages/alembic/script/base.py", line 1035, in _from_filename
    module = util.load_python_file(dir_, filename)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "venv/lib/python3.12/site-packages/alembic/util/pyfiles.py", line 93, in load_python_file
    module = load_module_py(module_id, path)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "venv/lib/python3.12/site-packages/alembic/util/pyfiles.py", line 109, in load_module_py
    spec.loader.exec_module(module)  # type: ignore
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap_external>", line 994, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "migrations/versions/be59ab2abe34_update_foreign_key_constraints.py", line 8, in <module>
    @dataclass
     ^^^^^^^^^
  File ".pyenv/versions/3.12.0/lib/python3.12/dataclasses.py", line 1266, in dataclass
    return wrap(cls)
           ^^^^^^^^^
  File ".pyenv/versions/3.12.0/lib/python3.12/dataclasses.py", line 1256, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".pyenv/versions/3.12.0/lib/python3.12/dataclasses.py", line 983, in _process_class
    and _is_type(type, cls, dataclasses, dataclasses.KW_ONLY,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".pyenv/versions/3.12.0/lib/python3.12/dataclasses.py", line 749, in _is_type
    ns = sys.modules.get(cls.__module__).__dict__
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute '__dict__'. Did you mean: '__dir__'?```

Versions.

  • OS: Linux
  • Python: 3.12.0
  • Alembic: 1.13.1
  • SQLAlchemy: 1.4.51
  • Database: n/a
  • DBAPI: n/a

Additional context
Found also other project suffering from this issue: mkdocs/mkdocs#3141

Have a nice day!

@kasium kasium added the requires triage New issue that requires categorization label Feb 9, 2024
@CaselIT
Copy link
Member

CaselIT commented Feb 9, 2024

Hi,

The linked issues seems to provide the solution, ie manually adding the module to the sys.modules dict.
I'm not sure if there is a reason for not adding them in alembic, @zzzeek do you know if it was left out on purpose?

@CaselIT CaselIT added bug Something isn't working execution model and removed requires triage New issue that requires categorization labels Feb 9, 2024
@CaselIT
Copy link
Member

CaselIT commented Feb 9, 2024

Interestingly when alembic still supported py27 this code actively removed the module from sys.modules in the py2 code:

if py3k:
import importlib.machinery
import importlib.util
def load_module_py(module_id, path):
spec = importlib.util.spec_from_file_location(module_id, path)
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
return module
def load_module_pyc(module_id, path):
spec = importlib.util.spec_from_file_location(module_id, path)
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
return module
def get_bytecode_suffixes():
try:
return importlib.machinery.BYTECODE_SUFFIXES
except AttributeError:
return importlib.machinery.DEBUG_BYTECODE_SUFFIXES
def get_current_bytecode_suffixes():
if py3k:
suffixes = importlib.machinery.BYTECODE_SUFFIXES
else:
if sys.flags.optimize:
suffixes = importlib.machinery.OPTIMIZED_BYTECODE_SUFFIXES
else:
suffixes = importlib.machinery.BYTECODE_SUFFIXES
return suffixes
def has_pep3147():
return True
else:
import imp
def load_module_py(module_id, path): # noqa
with open(path, "rb") as fp:
mod = imp.load_source(module_id, path, fp)
if py2k:
source_encoding = parse_encoding(fp)
if source_encoding:
mod._alembic_source_encoding = source_encoding
del sys.modules[module_id]
return mod
def load_module_pyc(module_id, path): # noqa
with open(path, "rb") as fp:
mod = imp.load_compiled(module_id, path, fp)
# no source encoding here
del sys.modules[module_id]
return mod
def get_current_bytecode_suffixes():
if sys.flags.optimize:
return [".pyo"] # e.g. .pyo
else:
return [".pyc"] # e.g. .pyc
def has_pep3147():
return False

But I guess if the current behaviour is wanted we could add the module to sys.modules, exec it, remove the module from sys.modules

@zzzeek
Copy link
Member

zzzeek commented Feb 9, 2024

Hi,

The linked issues seems to provide the solution, ie manually adding the module to the sys.modules dict. I'm not sure if there is a reason for not adding them in alembic, @zzzeek do you know if it was left out on purpose?

it is on purpose, since these aren't "modules" that are imported by "import", and I didnt think we wanted these python modules to live in the Python interpreter permanently until the interpreter ends..there could be hundreds of files taking up memory etc.

we do the same thing in mako FYI. so...might want to try over there too.

@zzzeek zzzeek changed the title Using dataclass and annotations future in migration leads to crash python dataclasses seems to look in sys.modules[__module__], so all dynamic loading patterns would need to add there too (which...how do you clean up ...shrugs) Feb 9, 2024
@CaselIT
Copy link
Member

CaselIT commented Feb 9, 2024

(which...how do you clean up ...shrugs)

well in python2 alembic was quite blunt about it del sys.modules[module_name]. Maybe it works also on python3?

Alternatively, since this seems a very rare case we do a "try current version; catch Attribute error and re-try with the module added in sys.modules"?

@zzzeek
Copy link
Member

zzzeek commented Feb 9, 2024

trying to clean up after the fact seems kind of messy

at the moment I feel like new config flag in alembic.ini that turns on "add to sys.modules ; note: migration files will remain persistent in the python interpreter until interpreter close unless removed manually" somethign like that

@CaselIT
Copy link
Member

CaselIT commented Feb 9, 2024

don't really like having yet another config knob, but it's probably the best option

@CaselIT CaselIT added this to the 1.13 milestone Feb 9, 2024
@zzzeek
Copy link
Member

zzzeek commented Feb 9, 2024

any chance someone reported this as a shortcoming in python dataclasses?

@CaselIT
Copy link
Member

CaselIT commented Feb 9, 2024

I think what python does here is very similar to what sqlalchemy does to evaluate a string annotation: exec it using the module dict to use the proper globals.

Not sure if anything different could be done in the dataclass module

@CaselIT
Copy link
Member

CaselIT commented Feb 14, 2024

honestly I'm not 100% it makes sense adding a new config flag just for this, since it's utility seems very low.

I'm for closing with "won't fix", since the workaround is simple: define the dataclass on another module and import it from the migration.

@CaselIT CaselIT added wontfix This will not be worked on and removed PRs (with tests!) welcome labels Feb 14, 2024
@zzzeek
Copy link
Member

zzzeek commented Feb 14, 2024

OK that's fine

@CaselIT CaselIT closed this as not planned Won't fix, can't repro, duplicate, stale Feb 14, 2024
@kasium
Copy link
Contributor Author

kasium commented Feb 14, 2024

Thanks for having a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution model wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants