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: Repository methods for repo CLI and other features #5156

Merged
merged 4 commits into from
Oct 4, 2021

Conversation

ramirezfranciscof
Copy link
Member

Methods added and modifications:

  • Added has_objects as new abstract and adapted has_object
  • Added delete_objects as new abstract and adapted delete_object
  • Added list_objects method
  • While modifying the inherited classes I removed some of the docstrings
    since these should be anyways inherited from the parent class and were
    just adding visual noise and maintenance costs.

@ramirezfranciscof
Copy link
Member Author

Created from #4965, @chrisjsewell I modified the type returned so let me know if this now works for you too or there is something else that would need adapting. In the meantime I'll try to add a couple of tests for these new features.

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #5156 (c2c64b1) into develop (2c5cc6f) will increase coverage by 0.02%.
The diff coverage is 95.72%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5156      +/-   ##
===========================================
+ Coverage    80.91%   80.93%   +0.02%     
===========================================
  Files          536      536              
  Lines        37019    37056      +37     
===========================================
+ Hits         29952    29987      +35     
- Misses        7067     7069       +2     
Flag Coverage Δ
django 75.78% <95.72%> (+0.03%) ⬆️
sqlalchemy 74.93% <95.72%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
aiida/backends/general/migrations/utils.py 86.39% <72.73%> (-1.11%) ⬇️
aiida/repository/backend/abstract.py 98.42% <100.00%> (+2.58%) ⬆️
aiida/repository/backend/disk_object_store.py 96.50% <100.00%> (+0.34%) ⬆️
aiida/repository/backend/sandbox.py 100.00% <100.00%> (ø)

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 2c5cc6f...c2c64b1. Read the comment docs.

@chrisjsewell
Copy link
Member

chrisjsewell commented Sep 29, 2021

Yep cheers, another thing from #5145 is that I think there should be a key_format property, e.g.

for the DiskObjectStoreRepositoryBackend it would be:

@property
def key_format(self):
	return self.container.hash_type

and for SandboxRepositoryBackend it would be:

@property
def key_format(self):
	return 'uuid4'

The reason this is necessary is that, when migrating between backends (e.g. archive -> main), one needs to know if the key_format are equal. If not, then you would need to re-compute all the Node.repository_metadata before importing them, otherwise they will not match with the repository.

@chrisjsewell
Copy link
Member

FYI, another thing I want to do, is to move Profile.get_repository to Backend.get_repository (see #5154)


@abc.abstractmethod
def list_objects(self) -> Iterable[str]:
"""Return iterable that yeilds all available objects by key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Return iterable that yeilds all available objects by key.
"""Return iterable that yields all available objects by key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@ramirezfranciscof ramirezfranciscof Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm developing inside a docker container, will this extension automatically install inside of it when I attach through the remote explorer or will I have to manually re-install it every time I shutdown and re-start the container?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know with the new vs code devcontainers, you can specify which extensions to load, see e.g. https://github.com/chrisjsewell/aiida-install/blob/56fe6d7398594c8813890652af99c521a77f249c/.devcontainer/devcontainer.json#L41

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you are using these you need to create the container through the vscode right? I'm not sure this would work for me because I need to initialize mine using docker-compose (I have 3 containers running for this: rabbitmq, postgres, and the work container).

Copy link
Member

@chrisjsewell chrisjsewell Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually playing around with that a bit but found, at least on my macbook, it was a bit slow a prone to the container dying:

.devcontainer/devcontainer.json

// For format details, see https://aka.ms/devcontainer.json. For config options, see the README at:
// https://github.com/microsoft/vscode-dev-containers/tree/v0.187.0/containers/docker-existing-dockerfile
// https://github.com/microsoft/vscode-dev-containers/tree/main/containers/python-3-postgres
{
    "name": "AiiDA development environment",
    "dockerComposeFile": "docker-compose.yml",
    "service": "main",
    "workspaceFolder": "/workspace",

	// Set *default* container specific settings.json values on container create.
	"settings": {
        "editor.rulers": [100],
        "python.pythonPath": "/usr/local/bin/python",
		"python.defaultInterpreterPath": "/usr/local/bin/python",
        "python.languageServer": "Pylance",
		"python.linting.enabled": true,
		"python.linting.pylintEnabled": true,
        "python.linting.pylintPath": "/usr/local/py-utils/bin/pylint",
        "python.formatting.provider": "yapf",
        "python.formatting.yapfPath": "/usr/local/py-utils/bin/yapf",
    },
	
	// Add the IDs of extensions you want installed when the container is created.
	"extensions": [
		"ms-python.python",
		"ms-python.vscode-pylance",
		"ms-azuretools.vscode-docker",
		"streetsidesoftware.code-spell-checker",
		"chrisjsewell.aiida-explore-vscode"
	],
}

.devcontainer/docker-compose.yml

version: '3.4'

services:

  main:
    image: 'mcr.microsoft.com/vscode/devcontainers/python:3.8'
    volumes:
      - ..:/workspace:cached
    # Overrides default command so things don't shut down after the process ends.
    command: sleep infinity

  postgres:
    image: postgres:12.3
    environment:
      POSTGRES_USER: pguser
      POSTGRES_PASSWORD: password
    restart: unless-stopped

  rmq:
    image: rabbitmq:3.8.3-management
    environment:
      RABBITMQ_DEFAULT_USER: guest
      RABBITMQ_DEFAULT_PASS: guest
    restart: unless-stopped

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, very interesting! Thanks, I will give this a try =D

ramirezfranciscof and others added 4 commits October 4, 2021 10:51
* Added has_objects as new abstract and apted has_object
* Added delete_objects as new abstract and adapted delete_object
* Added list_objects method
* While modifying the inherited classes I removed some of the docstrings
  since these should be anyways inherited from the parent class and were
  just adding visual noise and maintainance costs.
@ramirezfranciscof
Copy link
Member Author

I added the tests I could think of so this is now ready for another round of review. I also added the key_format property: if I understood correctly you only need the property defined here, not any feature using it, correct @chrisjsewell ?

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good thanks @ramirezfranciscof 👌

@ramirezfranciscof ramirezfranciscof merged commit 2a5d776 into aiidateam:develop Oct 4, 2021
chrisjsewell added a commit to chrisjsewell/aiida_core that referenced this pull request Oct 8, 2021
This is essentially an addition to aiidateam#5156 and required aiidateam#5145

Without the "optimised" use of `Container.get_objects_stream_and_meta` for the `DiskObjectStoreRepositoryBackend`, the profiled archive creation in aiidateam#5145 goes from 4 minutes to 9 minutes!
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