AEP: Abstract and improve the node file repository #7

sphuber · 2019-10-18T13:40:33Z

ltalirz

Thanks a lot @sphuber !
Below a few comments and suggestions.

001_file_repository_improvement/readme.md

sphuber · 2019-10-19T09:41:42Z

Below a few comments and suggestions.

@ltalirz what is the typical procedure for an AEP PR? If I want to apply the changes I just make a new commit and push? Do we want to keep the commit history here? What is the typical approach?

ltalirz · 2019-10-19T11:40:26Z

@ltalirz what is the typical procedure for an AEP PR? If I want to apply the changes I just make a new commit and push?

Yes, that's the idea.

Do we want to keep the commit history here? What is the typical approach?

I'd say let's keep the commit history at least in the PR.
When merging, we can decide to squash it depending on the (e.g. have a look a the commit history over at the jupyter folks).

Feel free to add me as a "champion" - then we should ask for reviews from all relevant parties, if you think you can even mention the @aiidateam/plugin-developers

For reference, here is how jupyter propose to handle this review period

greschd · 2019-10-23T13:37:13Z

I think this would be a good opportunity to also get rid of empty repository folders: The NodeRepositoryManager could be lazy in that it only creates a {folder, remote resource} when something is actually put in there.

The motivation for this is that FS performance (things like ls, du, find etc.) is impacted greatly by the number of folders in the repository, even if only ~1% of nodes actually contain data.

001_file_repository_improvement/readme.md

chrisjsewell · 2021-05-06T08:08:38Z

@sphuber, as discussed, perhaps this should be marked as rejected (at least for the exact spec you outlined here) and then merged/closed (?), and maybe you could add a note of why this was the case

sphuber · 2021-12-16T15:27:05Z

@ltalirz @chrisjsewell @giovannipizzi I have updated the AEP to reflect the current implementation in aiida-core. I essentially rewrote the whole text, so all old comments no longer apply.

ltalirz

thanks for taking the time to wrap this up @sphuber !

I haven't had the time to read through everything but since the implementation is already done I think it's great to have the thought process documented here.

Good to go from my side!

chrisjsewell · 2021-12-16T16:34:21Z

just having a read...

chrisjsewell

few nitpicks

chrisjsewell · 2021-12-16T16:50:56Z

007_improved_file_repository/readme.md

+
+## Proposed Enhancement
+The scaling problem of the original file repository implementation has already been described in great detail in [AEP 006: Efficient object store for the AiiDA repository](https://github.com/aiidateam/AEP/tree/master/006_efficient_object_store_for_repository#readme).
+It also describes an effecient key-value store that is implemented in the [`disk-objectstore`](https://github.com/aiidateam/disk-objectstore) library.


Suggested change

It also describes an effecient key-value store that is implemented in the [`disk-objectstore`](https://github.com/aiidateam/disk-objectstore) library.

It also describes an efficient key-value store that is implemented in the [`disk-objectstore`](https://github.com/aiidateam/disk-objectstore) library.

chrisjsewell · 2021-12-16T16:52:44Z

007_improved_file_repository/readme.md

+        :raise OSError: if the file could not be deleted.
+        """
+```
+Since it is envisioned that solutions will be implemented that are not necessarily file system based, it is important that the interface does not implicitcly bake-in file-system-only concepts.


Suggested change

Since it is envisioned that solutions will be implemented that are not necessarily file system based, it is important that the interface does not implicitcly bake-in file-system-only concepts.

Since it is envisioned that solutions will be implemented that are not necessarily file system based, it is important that the interface does not implicitly bake-in file-system-only concepts.

chrisjsewell · 2021-12-16T17:12:03Z

007_improved_file_repository/readme.md

+
+## Detailed Explanation
+
+### Abstracting the file repository interface


I think it would be great here to have a basic before & after UML of this, similar (but not necessarily as detailed) to what I have done in aiidateam/aiida-core#5172 (comment),
so people not already intimately knowledgable with AiiDA can get a quick idea of whats going on

Would this image I made for the documentation fulfill that need?

yep perfect 👍 Perhaps if you also had a "before" one as well

Don't have a before, and there is not much to describe really. The code was more or less directly hard coded in the Node class.

chrisjsewell · 2021-12-16T19:09:59Z

README.md

+| 004    | implemented      | [Infrastructure for importing completed `CalcJob`s](004_calcjob_importer/readme.md)                    |
+| 005    | draft            | [New Export Format](005_exportformat/readme.md)                                                        |
+| 006    | implemented      | [Efficient object store for the AiiDA repository](006_efficient_object_store_for_repository/readme.md) |
+| 007    | implemented      | [Abstract and improve the file repository](007_improved_file_repository/)                              |


Technically not a revert, since it is a new row. But I see the problem, just accidentally forgot.

Fixed, guvnor

sphuber added the status/submitted label Oct 18, 2019

ltalirz reviewed Oct 18, 2019

View reviewed changes

sphuber mentioned this pull request Oct 20, 2019

Add raw parser of Lowdin charges from projwfc output file aiidateam/aiida-quantumespresso#425

Closed

chrisjsewell reviewed May 6, 2021

View reviewed changes

001_file_repository_improvement/readme.md Outdated Show resolved Hide resolved

giovannipizzi mentioned this pull request Dec 10, 2021

Efficient object store for the AiiDA repository #11

Merged

5 tasks

sphuber added the type/standard Standard Track AEP label Dec 16, 2021

sphuber changed the title ~~Abstract and improve the node repository~~ AEP: Abstract and improve the node file repository Dec 16, 2021

sphuber force-pushed the aep_repository branch from ff12f6f to 81a71db Compare December 16, 2021 15:25

sphuber requested review from ltalirz, chrisjsewell and giovannipizzi December 16, 2021 15:26

ltalirz approved these changes Dec 16, 2021

View reviewed changes

chrisjsewell approved these changes Dec 16, 2021

View reviewed changes

sphuber force-pushed the aep_repository branch 4 times, most recently from 570c899 to fe4801c Compare December 16, 2021 18:29

chrisjsewell reviewed Dec 16, 2021

View reviewed changes

AEP: Abstract and improve the node file repository

f222840

sphuber force-pushed the aep_repository branch from fe4801c to f222840 Compare December 16, 2021 20:47

sphuber merged commit f178459 into aiidateam:master Dec 16, 2021

sphuber deleted the aep_repository branch December 16, 2021 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AEP: Abstract and improve the node file repository #7

AEP: Abstract and improve the node file repository #7

sphuber commented Oct 18, 2019 •

edited

Loading

ltalirz left a comment

sphuber commented Oct 19, 2019 •

edited

Loading

ltalirz commented Oct 19, 2019

greschd commented Oct 23, 2019

chrisjsewell commented May 6, 2021

sphuber commented Dec 16, 2021

ltalirz left a comment

chrisjsewell commented Dec 16, 2021

chrisjsewell left a comment

chrisjsewell Dec 16, 2021

chrisjsewell Dec 16, 2021

chrisjsewell Dec 16, 2021

sphuber Dec 16, 2021

chrisjsewell Dec 16, 2021

sphuber Dec 16, 2021

chrisjsewell Dec 16, 2021

sphuber Dec 16, 2021

sphuber Dec 16, 2021

	It also describes an effecient key-value store that is implemented in the [`disk-objectstore`](https://github.com/aiidateam/disk-objectstore) library.
	It also describes an efficient key-value store that is implemented in the [`disk-objectstore`](https://github.com/aiidateam/disk-objectstore) library.

	Since it is envisioned that solutions will be implemented that are not necessarily file system based, it is important that the interface does not implicitcly bake-in file-system-only concepts.
	Since it is envisioned that solutions will be implemented that are not necessarily file system based, it is important that the interface does not implicitly bake-in file-system-only concepts.


		## Detailed Explanation

		### Abstracting the file repository interface

	\| 007 \| implemented \| [Abstract and improve the file repository](007_improved_file_repository/) \|
	\| 007 \| implemented \| [Abstract and improve the file repository](007_improved_file_repository/readme.md) \|

AEP: Abstract and improve the node file repository #7

AEP: Abstract and improve the node file repository #7

Conversation

sphuber commented Oct 18, 2019 • edited Loading

ltalirz left a comment

Choose a reason for hiding this comment

sphuber commented Oct 19, 2019 • edited Loading

ltalirz commented Oct 19, 2019

greschd commented Oct 23, 2019

chrisjsewell commented May 6, 2021

sphuber commented Dec 16, 2021

ltalirz left a comment

Choose a reason for hiding this comment

chrisjsewell commented Dec 16, 2021

chrisjsewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented Oct 18, 2019 •

edited

Loading

sphuber commented Oct 19, 2019 •

edited

Loading