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

Inconsistencies in Model Registration and Unregistration Functions in models.py #6677

Closed
Geogouz opened this issue Apr 14, 2024 · 1 comment
Assignees
Milestone

Comments

@Geogouz
Copy link
Contributor

Geogouz commented Apr 14, 2024

I noticed couple of inconsistencies in the models.py related to how models are registered and unregistered.

For example:

  1. Duplicate Entries: The model ReplicationRule is listed twice in the register_models function (even though SQLAlchemy shouldn't have a problem handling this).

ReplicaLock,
ReplicationRule,
ReplicationRule,
ReplicationRuleHistory,

  1. Missing Model in Unregistration Function: The VirtualPlacements model is included in the register_models function but is absent from the unregister_models function.

  2. Missing Models from both Functions: The models BadPFNs, TransferLimit, RSETransferLimit, DatasetLock, Distance, TransferStats are not considered at all.

Are (any of) these intensional?

@erlingstaff
Copy link
Contributor

erlingstaff commented Apr 22, 2024

The current implementation is very confusing and not ideal. Even though there are models missing in register and unregister, as you noted, they all get created and deleted correctly (the commands in psql screenshot are run before/after running ./tools/destroy_database.py, which calls unregister_models).

Pasted Graphic 1

When unregister or register_models is run now, it loops over all the listed models and calls model.metadata.drop_all(engine), where metadata is a db schema, from which all tables (models) are dropped. However, they all inherit the same metadata and schema from the BASE class.

TL;DR:
Currently register_models tries to register the same schema many times (one time for each model listed), likewise with unregister_models. The schema contains all the models. Simplified down to a single call BASE.metadata.drop_all(engine) / BASE.metadata.create_all(engine) to register or unregister the schema. #6712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants