`Profile`: do not leak repository implementation in interface #4891

sphuber · 2021-04-29T11:22:05Z

The Profile interface was leaking implementation details of the
repository. For example, the method get_repository_container returned
the Container class of the disk-objectstore library, but this is an
implementation detail. If this ever were to change all client code would
break.

Instead, we add the get_repository method which returns an instance of
the generic Repository class that will be common to all repository
implementations. The backend implementation can still be obtained
through this object but this should only be done in exceptional cases.

The Repository class now has three additional properties and methods:

def uuid(self) -> typing.Optional[str]:
def initialise(self, **kwargs) -> None:
def is_initialised(self) -> bool:

These simply call through to the exact same method/property on the
backend instance. The AbstractRepositoryBackend also now has the exact
same attributes and they are implemented for the two currently existing
implementations DiskObjectStoreRepositoryBackend and the
SandboxRepositoryBackend.

sphuber · 2021-04-29T11:22:57Z

Haven't gone over all the comments of @CasperWA of the other PR yet, but will do later. I propose @mbercx already goes over the suggested interface changes here. Will have time to get back to this later in the afternoon.

chrisjsewell

well perhaps I should say, approved after it doesn't break all the tests lol

CasperWA · 2021-04-29T11:28:48Z

Haven't gone over all the comments of @CasperWA of the other PR yet, but will do later (...)

They're all syntax and style, essentially. So no worries.

aiida/manage/configuration/profile.py

mbercx

Thanks @sphuber. Gave the PR a first pass.

aiida/manage/configuration/profile.py

aiida/manage/manager.py

aiida/manage/configuration/profile.py

sphuber · 2021-04-29T13:48:18Z

Alright, this should be much better. If you guys are happy with the design, I will add the necessary tests for the new Repository and RepositoryBackend properties and methods.

mbercx · 2021-04-29T13:50:29Z

Alright, this should be much better. If you guys are happy with the design, I will add the necessary tests for the new Repository and RepositoryBackend properties and methods.

Looking very good indeed! Much cleaner to have these methods on the Repository class. 👌 Will do a proper review and some testing after the wrap-up meeting.

chrisjsewell · 2021-04-29T13:52:14Z

Looking very good indeed!

not that it means much to have @mbercx 's seal of a approval

CasperWA · 2021-04-29T13:53:35Z

Looking very good indeed!

not that it means much to have @mbercx 's seal of a approval

Indeed. I quickly jumped in to review when I saw I blanketed this PR ... tsk tsk tsk... 😂

mbercx · 2021-04-29T14:02:47Z

Looking very good indeed!

not that it means much to have @mbercx 's seal of a approval

Does that mean I don't have to review PR's anymore? 🥳

aiida/cmdline/commands/cmd_status.py

aiida/repository/backend/sandbox.py

aiida/repository/repository.py

tests/cmdline/commands/test_setup.py

CasperWA · 2021-04-29T14:07:45Z

Looking very good indeed!

not that it means much to have @mbercx 's seal of a approval

Does that mean I don't have to review PR's anymore? 🥳

Nah. Just means more work for the rest of us 😂

sphuber · 2021-04-29T15:55:50Z

Ok, the docs are failing with the following:

/home/circleci/.local/lib/python3.7/site-packages/aiida/manage/configuration/profile.py:docstring of aiida.manage.configuration.Profile.get_repository:: WARNING: py:class reference target not found: Repository
/home/circleci/.local/lib/python3.7/site-packages/aiida/manage/configuration/profile.py:docstring of aiida.manage.configuration.profile.Profile.get_repository:: WARNING: py:class reference target not found: Repository

This is most likely becuase of the return type, which I put as a string. The reason being that I cannot import Repository at the top of the module, because this leads to a circular import. For the typing, I put a guard in if typing.TYPE_CHECKING which makes sure mypy can properly import it, but for the docs this doesn't work. Not sure how to get around this. Any ideas? Other than maybe trying to break the circular import in the aiida.repository.Repository path, but that might not be trivial.

chrisjsewell · 2021-04-30T07:52:33Z

This is most likely becuase of the return type, which I put as a string.

yeh its an annoying sphinx thing (sphinx-doc/sphinx#8498), just add it to the docs/source/nitpick-exceptions

The `Profile` interface was leaking implementation details of the repository. For example, the method `get_repository_container` returned the `Container` class of the `disk-objectstore` library, but this is an implementation detail. If this ever were to change all client code would break. Instead, we add the `get_repository` method which returns an instance of the generic `Repository` class that will be common to all repository implementations. The backend implementation can still be obtained through this object but this should only be done in exceptional cases. The `Repository` class now has three additional properties and methods: * `def uuid(self) -> typing.Optional[str]:` * `def initialise(self, **kwargs) -> None:` * `def is_initialised(self) -> bool:` These simply call through to the exact same method/property on the backend instance. The `AbstractRepositoryBackend` also now has the exact same attributes and they are implemented for the two currently existing implementations `DiskObjectStoreRepositoryBackend` and the `SandboxRepositoryBackend`.

codecov · 2021-04-30T08:15:24Z

Codecov Report

Merging #4891 (8145703) into develop (975affe) will increase coverage by 0.03%.
The diff coverage is 90.55%.

@@             Coverage Diff             @@
##           develop    #4891      +/-   ##
===========================================
+ Coverage    80.10%   80.12%   +0.03%     
===========================================
  Files          517      517              
  Lines        36617    36659      +42     
===========================================
+ Hits         29327    29368      +41     
- Misses        7290     7291       +1

Flag	Coverage Δ
django	`74.58% <89.19%> (+0.01%)`	⬆️
sqlalchemy	`73.53% <89.19%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/cmdline/commands/cmd_setup.py	`49.13% <0.00%> (ø)`
aiida/manage/manager.py	`81.36% <0.00%> (+0.91%)`	⬆️
aiida/backends/general/migrations/utils.py	`88.89% <62.50%> (-0.97%)`	⬇️
aiida/repository/backend/sandbox.py	`98.04% <91.67%> (-1.96%)`	⬇️
aiida/manage/configuration/profile.py	`95.06% <92.31%> (-0.05%)`	⬇️
...ds/djsite/db/migrations/0047_migrate_repository.py	`70.97% <100.00%> (ø)`
...ations/versions/1feaea71bd5a_migrate_repository.py	`81.14% <100.00%> (ø)`
aiida/cmdline/commands/cmd_status.py	`74.74% <100.00%> (ø)`
aiida/orm/nodes/node.py	`96.03% <100.00%> (-0.02%)`	⬇️
aiida/orm/nodes/repository.py	`93.59% <100.00%> (-0.08%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 975affe...8145703. Read the comment docs.

sphuber · 2021-04-30T08:44:28Z

@mbercx would you still like a gander before we merge?

mbercx · 2021-04-30T08:57:07Z

@mbercx would you still like a gander before we merge?

LGMT! 🚀

approved

sphuber requested a review from mbercx April 29, 2021 11:22

chrisjsewell approved these changes Apr 29, 2021

View reviewed changes

chrisjsewell suggested changes Apr 29, 2021

View reviewed changes

chrisjsewell reviewed Apr 29, 2021

View reviewed changes

aiida/manage/configuration/profile.py Outdated Show resolved Hide resolved

mbercx previously requested changes Apr 29, 2021

View reviewed changes

aiida/manage/configuration/profile.py Outdated Show resolved Hide resolved

aiida/manage/manager.py Outdated Show resolved Hide resolved

aiida/manage/configuration/profile.py Outdated Show resolved Hide resolved

sphuber force-pushed the fix/4890/profile-repository-interface branch 2 times, most recently from 00bb1a0 to 1934981 Compare April 29, 2021 13:41

sphuber requested review from chrisjsewell, CasperWA and mbercx April 29, 2021 13:47

CasperWA reviewed Apr 29, 2021

View reviewed changes

sphuber force-pushed the fix/4890/profile-repository-interface branch 2 times, most recently from aab092a to 04cd181 Compare April 29, 2021 15:21

sphuber force-pushed the fix/4890/profile-repository-interface branch from 04cd181 to 8145703 Compare April 30, 2021 07:58

chrisjsewell approved these changes Apr 30, 2021

View reviewed changes

sphuber merged commit fa8d05f into aiidateam:develop Apr 30, 2021

sphuber deleted the fix/4890/profile-repository-interface branch April 30, 2021 09:03

CasperWA mentioned this pull request Apr 30, 2021

Cannot migrate due to container folder being present #4897

Closed

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Profile`: do not leak repository implementation in interface #4891

`Profile`: do not leak repository implementation in interface #4891

sphuber commented Apr 29, 2021 •

edited

Loading

sphuber commented Apr 29, 2021

chrisjsewell left a comment

CasperWA commented Apr 29, 2021

mbercx left a comment

sphuber commented Apr 29, 2021

mbercx commented Apr 29, 2021

chrisjsewell commented Apr 29, 2021

CasperWA commented Apr 29, 2021

mbercx commented Apr 29, 2021

CasperWA commented Apr 29, 2021

sphuber commented Apr 29, 2021

chrisjsewell commented Apr 30, 2021

codecov bot commented Apr 30, 2021

sphuber commented Apr 30, 2021

mbercx commented Apr 30, 2021

Profile: do not leak repository implementation in interface #4891

Profile: do not leak repository implementation in interface #4891

Conversation

sphuber commented Apr 29, 2021 • edited Loading

sphuber commented Apr 29, 2021

chrisjsewell left a comment

Choose a reason for hiding this comment

CasperWA commented Apr 29, 2021

mbercx left a comment

Choose a reason for hiding this comment

sphuber commented Apr 29, 2021

mbercx commented Apr 29, 2021

chrisjsewell commented Apr 29, 2021

CasperWA commented Apr 29, 2021

mbercx commented Apr 29, 2021

CasperWA commented Apr 29, 2021

sphuber commented Apr 29, 2021

chrisjsewell commented Apr 30, 2021

codecov bot commented Apr 30, 2021

Codecov Report

sphuber commented Apr 30, 2021

mbercx commented Apr 30, 2021

`Profile`: do not leak repository implementation in interface #4891

`Profile`: do not leak repository implementation in interface #4891

sphuber commented Apr 29, 2021 •

edited

Loading