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 meta class to handle config saving #591

Merged
merged 9 commits into from
Sep 23, 2023

Conversation

AMHermansen
Copy link
Collaborator

@AMHermansen AMHermansen commented Sep 13, 2023

Currently we're using a decorator around every __init__ of classes that inherits from Model/Dataset. This creates redundant boilerplate code. This PR aims to introduce a new way of handling the backend config saving. This is done by creating metaclasses responsible for intercepting the objects created and store a corresponding config dict to the created objects.

The following is done:

  1. Create relevant metaclasses (ModelConfigSaverMeta, DatasetConfigSaverMeta) along with common interfaces between these metaclasses and ABCMeta.
  2. Remove usage of save_model_config and save_dataset_config and add deprecation warnings when they're used in code.

This means the config saving is purely done in the backend, away from any frontend users, making the code easier to read and avoids potential slip-ups where one might forget to use @save_model_config.

This PR currently introduces a bug. I suspect the bug is a conflict with #584.

Could you maybe try to take a look at this @RasmusOrsoe

…cation warning. To not break backwards compatibility.
@AMHermansen
Copy link
Collaborator Author

AMHermansen commented Sep 14, 2023

Previously mentioned bug was resolved by #593, wasn't an issue with this PR.

@AMHermansen AMHermansen marked this pull request as ready for review September 14, 2023 09:35
@AMHermansen
Copy link
Collaborator Author

AMHermansen commented Sep 14, 2023

mypy is only failing for code, which wasn't modified by this PR.
I believe the source of this, is that the decorator save_model_configs alters the expected argument types for __init__ to be *args: Any, **kwargs: Any. Which means all __init__ functions trivially passes the mypy test. Fixing those seems out of scope for this PR, and should probably be adressed in a separate PR (possibly before merging this).

@RasmusOrsoe
Copy link
Collaborator

@AMHermansen Let's take a look at this together after the dev meeting, if you have time.

@AMHermansen
Copy link
Collaborator Author

@RasmusOrsoe I've now changed the typehints from the source code, that was conflicting with mypy (except for the SQLitePertubedDataset). This means that currently it is only the examples which are failing for mypy, feel free to have a look at this PR when you get the time 😄 .

@RasmusOrsoe RasmusOrsoe merged commit 0b73f6f into graphnet-team:main Sep 23, 2023
11 of 12 checks passed
@AMHermansen AMHermansen deleted the add-ConfSaverMeta branch September 29, 2023 08:05
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.

2 participants