[WIP] Remove datastore arg from models since it is already in the config file #117
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
So this is a WIP MR, and I'm not sure we are interested in this design at all, I just made the MR to save the work for later if we want to pick it up.
I've been looking randomly through the codebase to see how everything is working together - I noticed that instantiating the model is quite hard—see this example. This complexity makes it difficult to test and reuse the model outside of the main training script.
Issue
There is some redundancy in how the model datastores are passed to the model during instantiation. Specifically, the model:
Accepts a NeuralLAMConfig, which includes both datastore_path and datastore_type.
Also takes an already instantiated datastore object as an argument.
This means the model effectively gets the same information from two different sources, leading to unnecessary complexity.
Proposed Change
To streamline instantiation, I removed the datastore from the model's arguments and instead let the model instantiate it internally based on the provided config. This should reduce redundancy in argument passing and make it easier to instantiate the model for usage in different scripts.
Thoughts
I think that the model should not have a datastore as a member since it introduces a tight coupling between model and data and is against the core modular philosophy of neurallam
No change of dependencies
Issue Link
< Link to the relevant issue or task. > (e.g.
closes #00
orsolves #00
)Type of change
Checklist before requesting a review
pull
with--rebase
option if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee