Deprecate Profile.repository_path
#5516
Conversation
sphuber
force-pushed
the
fix/5513/deprecate-profile-repository-path
branch
3 times, most recently
from
1bdb698
to
e1a3cb8
Compare
Mmm I understand the technical rationale behind this, but I feel like it is a bit obfuscating to have the implementation in the Probably this is not the ideal solution, but as an alternative that at least does not make the interweaving of these more complex, could we maybe pass the |
That's what I tried initially. I tried to decouple this and make the migrator take an instance of the backend, but that won't work for now as the backend needs to be validated as it is constructed, for which it needs the migrator, and so there is a circular dependency. I discussed with @chrisjsewell and for now there is not better quick solution without a big refactoring of the backend/migrator.
Don't think this will work because if you look at the code, it is not just in |
|
Right, I was not suggesting you do the refactoring now, but perhaps it would be a good time to open an issue documenting a bit the intricacy of the problem since you have just clashed with it and have the details fresh in your head. In any case, I think I'll remove myself from the reviewers because it seems a lot of contextual information about these classes is needed to do a good review of the changes. |
There was a problem hiding this comment.
Sorry, but there is no way I'm excepting this in the current form.
Among other things, make filepath_container a static/standalone function, there is absolutely no need for it to be dependent on instances of the Migrator or StorageBackend, it only depends on the profile
How so? Its implementation comes from the |
Because you are massively overcomplicating a simple function that turns a string into a path, and passing around classes where they don't need to be passed |
sphuber
force-pushed
the
fix/5513/deprecate-profile-repository-path
branch
from
e1a3cb8
to
0b9cc23
Compare
|
There @chrisjsewell I have turned it into a function. Should be good now |
sphuber
force-pushed
the
fix/5513/deprecate-profile-repository-path
branch
2 times, most recently
from
677bd7d
to
b82351a
Compare
|
Want to still review this @chrisjsewell ? Otherwise I'll merge it |
sphuber
force-pushed
the
fix/5513/deprecate-profile-repository-path
branch
from
b82351a
to
629a123
Compare
The `Profile.repository_path` was historically used to give the absolute filepath to the file repository on the local file system. However, this is a property of the storage backend and not all backends will have this. So instead of being generic to all profiles, this should be a storage specific setting. The property is deprecated and the functionality is added to a new function `aiida.storage.psql_dos.backend.get_filepath_container`. We considered adding it as a property on the `PsqlDosBackend` class, which makes the most sense semantically, but that made the code more complex than ideal. The reason for this is because the property needs to be accessed both from the backend as well as the associated migrator. However, the migrator cannot have a reference to the backend since the migrator is used to validate the backend class in its constructor, which would lead to a circular dependence. The alternative of implementing the property on the migrator and exposing it on the backend was possible, but also slightly convoluted. The root of this problem is the limitation in the current design that the migrator cannot have a reference to the storage backend to which it belongs, but this should be addressed in another commit. There is a also a slight difference in the implementation of the new function. It returns the path of the disk object store container, which is a subfolder of the original repository path. Old invocations of `profile.repository_path` should therefore be replaced with `get_filepath_container(profile).parent`.
sphuber
force-pushed
the
fix/5513/deprecate-profile-repository-path
branch
from
1ed234b
to
dc16d11
Compare
Fixes #5513
The
Profile.repository_pathwas historically used to give the absolutefilepath to the file repository on the local file system. However, this
is a property of the storage backend and not all backends will have this.
So instead of being generic to all profiles, this should be a storage
specific setting.
The property is deprecated and the functionality is added to the new
PsqlDosBackend.filepath_container. This forwards to the same propertybut on the
PsqlDostoreMigratorthat has the actual implementation. Thereason for this is because the migrator needs to get the repository
location in the
migratemethod, but that needs to remain aclassmethod. The alternative of having the migrator call the property on
the backend, would require the backend to be passed to the constructor
of the migrator, but this won't work as the migrator also needs to be
constructed in the constructor of the backend, leading to an infinite
regression.
There is a also a slight difference in the implementation of the new
property. It returns the path of the disk object store container, which
is a subfolder of the original repository path. Old invocations of
profile.repository_pathshould therefore be replaced withbackend.filepath_container.parent.Currently based on and blocked by #5496