Node post methods #25

NinadBhat · 2021-06-15T02:37:39Z

No description provided.

NinadBhat · 2021-06-17T07:22:53Z

@ltalirz, @chrisjsewell any idea about psycopg2.errors.ActiveSqlTransaction: CREATE DATABASE cannot run inside a transaction block? Not sure about the reason behind this error.

NinadBhat · 2021-06-17T12:58:01Z

Failure related to aiidateam/aiida-core#4989

ltalirz · 2021-06-17T22:39:25Z

Hi @NinadBhat , sorry for the late reply - do I assume correctly that this is fixed by the changes in the PR (i.e. preventing psycopg2-binary from upgrading to 2.9)?

NinadBhat · 2021-06-17T22:49:55Z

@ltalirz I have not sure where the psycopg2-binary version is set.

chrisjsewell · 2021-06-17T22:52:49Z

It won’t magically fix it here, or for any plugin at the moment I’m afraid, since it is only on the develop branch. You will need to specifically pin it in the setup.json here for now (that’s what I’ve done for aiida-lamps)

ltalirz · 2021-06-17T22:53:12Z

I guess the tests are passing locally for you but not on CI since there it will still download the previous AiiDa release (which does not prevent psycopg2-binary 2.9 from being installed).
Until a fixed aiida version is released, simply add psycopg2-binary~=2.8.3 here in the setup.json

codecov-commenter · 2021-06-18T00:16:28Z

Codecov Report

Merging #25 (0f78774) into master (35ea00f) will increase coverage by 0.81%.
The diff coverage is 98.31%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   92.24%   93.06%   +0.81%     
==========================================
  Files          25       26       +1     
  Lines         761      879     +118     
==========================================
+ Hits          702      818     +116     
- Misses         59       61       +2

Flag	Coverage Δ
pytests	`93.06% <98.31%> (+0.81%)`	⬆️

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

Impacted Files	Coverage Δ
aiida_restapi/routers/nodes.py	`95.83% <95.83%> (ø)`
aiida_restapi/main.py	`100.00% <100.00%> (ø)`
aiida_restapi/models.py	`97.85% <100.00%> (+1.96%)`	⬆️
aiida_restapi/routers/groups.py	`100.00% <100.00%> (ø)`
aiida_restapi/routers/users.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 35ea00f...0f78774. Read the comment docs.

NinadBhat · 2021-06-22T00:15:08Z

@chrisjsewell, @ltalirz
For File Upload, I have used File class for fastapi [1]. One limitation I encountered was that you cant post JSON data along with the file parameter (it's a limitation of the HTTP protocol). So I used Form Data for posting details as about node_type, attributes, etc. The code for verification of form data with pydantic model was taken from fastapi/fastapi#2387.

ltalirz

thanks @NinadBhat , looking good!

just a couple of things to fix
Since this is all new functionality, I would suggest you also add add some point a bit of documentation to the docs/ folder.
It doesn't have to be in this PR, but when you get to the processes I would suggest to simply document the steps of submitting a new workflow (including creation of inputs, checking status, etc.) from start to finish.

ltalirz · 2021-07-19T08:44:11Z

aiida_restapi/models.py

+ ) -> orm.Node:
+ "Create and Store new Node"
+ orm_object = load_entry_point_from_full_type(node_type)(**node_dict)
+ orm_object.set_attribute_many(attributes)


extras missing?

ltalirz · 2021-07-19T08:45:43Z

aiida_restapi/models.py

+ orm_object = load_entry_point_from_full_type(node_type)(
+ file=io.BytesIO(file), **node_dict
+ )
+ orm_object.set_attribute_many(attributes)


ltalirz · 2021-07-19T08:47:18Z

aiida_restapi/routers/nodes.py

+from aiida.cmdline.utils.decorators import with_dbenv
+from fastapi import APIRouter, Depends, File
+
+from aiida_restapi.models import Node, User


maybe rather
from aiida_restapi import models
so that you can later user models.Node in the code (this makes it clear that you are referring to models there, not e.g. ORM objects)

do you agree?

ltalirz · 2021-07-19T08:48:14Z

aiida_restapi/routers/nodes.py

+
+ node_dict = node.dict(exclude_unset=True)
+ node_type = node_dict.pop("node_type", None)
+ attributes = node_dict.pop("attributes", None)


won't the extra be set when passing parameter using **node_dict?

I see, maybe - can you please add a test?

If this works fine, then perhaps it would also make sense not to treat the attributes in a special way here since you aren't doing anything specific with them.
I.e. perhaps just leave them in the node_dict and let the create_new_node method deal with popping out the attributes instead?

@ltalirz You were correct about extras, I have added extras and test for extras.

ltalirz · 2021-07-19T08:53:57Z

aiida_restapi/routers/nodes.py

+ for ep_node in entry_point_nodes:
+ if ep_node.name == node_type:
+ orm_object = ep_node.load().create_new_node(
+ node_type, attributes, node_dict
+ )


this is not optimal

no exception here if entry point string not found

what if the entry point string was found twice? (ok, should not happen in theory...)

no need to continue after entry point is found

I would suggest to first check whether instead of a list you can get a dictionary "entry point string" => "entry point object" from the API.
If not, you will need to put some guards in here, e.g. break after entry point is found; after the look check whether it was found and raise if it wasn't

Any updates on this?

ltalirz · 2021-07-19T08:55:02Z

aiida_restapi/routers/nodes.py

+ """Test function for upload file will be merged with create_node later."""
+ node_dict = params.dict(exclude_unset=True, exclude_none=True)
+ node_type = node_dict.pop("node_type", None)
+ attributes = node_dict.pop("attributes", {})


ltalirz · 2021-07-19T09:00:27Z

setup.json

+ "aiida.rest.post": [
+ "data.str.Str.| = aiida_restapi.models:Node",
+ "data.float.Float.| = aiida_restapi.models:Node",
+ "data.int.Int.| = aiida_restapi.models:Node",
+ "data.bool.Bool.| = aiida_restapi.models:Node",
+ "data.structure.StructureData.| = aiida_restapi.models:Node",
+ "data.orbital.OrbitalData.| = aiida_restapi.models:Node",
+ "data.list.List.| = aiida_restapi.models:Node",
+ "data.dict.Dict.| = aiida_restapi.models:Node",
+ "data.singlefile.SingleFileData.| = aiida_restapi.models:Node"


Specifying entry points in the setup.json/setup.py is the right approach for AiiDA plugins but here it is not ideal since we are adding entry points here that depend on a different package (aiida-core).
I.e. this list would need to change whenever AiiDA adds a new data node type.

It seems to be possible to add entry points dynamically, see
https://stackoverflow.com/a/48666503/1069467
This uses pkg_resources- perhaps there is a way to do it with importlib as well (not necessarily; if I remember well, in your first PR to AiiDA you already encountered an issue where importlib did lack some of the functionality of the old pkg_resources)

In any case, this does seem like a use case for dynamically adding entry points (i.e. it could be done in the __init__.py of aiida_restapi)

I guess this list can be removed now?

Didn't we discuss that we would merge things between the full_type strings known to AiiDa and entrypoints that could be added here?

ltalirz · 2021-07-19T09:06:01Z

tests/test_nodes.py

+ json={
+ "node_type": "data.dict.Dict.|",
+ "attributes": {"x": 1, "y": 2},
+ "label": "test_dict",


please add a test that checks you get an error if you provide attributes that cannot be set by the user (e.g. uuid, mtime, ...)

have you added it? (sorry if I'm overlooking it)

ltalirz · 2021-07-19T09:08:30Z

tests/test_nodes.py

+): # pylint: disable=unused-argument
+ """Testing file upload"""
+ test_file = {
+ "upload_file": (


is this a hardcoded string?

ltalirz · 2021-07-24T09:45:41Z

Hi @NinadBhat , are you going to address the points raised?

ltalirz

hey @NinadBhat , I just had another look at the PR but it seems there were a couple of comments from my previous review that were not yet addressed?
Could you please look into those?

ltalirz · 2021-08-14T20:27:56Z

aiida_restapi/routers/nodes.py

+ for ep_node in entry_point_nodes:
+ if ep_node.name == node_type:
+ orm_object = ep_node.load().create_new_node(
+ node_type, attributes, node_dict
+ )


Any updates on this?

ltalirz

thanks @NinadBhat , let's get this merged

ltalirz · 2021-08-18T10:32:49Z

aiida_restapi/routers/nodes.py

+ get_current_active_user
+ ), # pylint: disable=unused-argument
+) -> models.Node:
+ """Test function for upload file will be merged with create_node later."""


Ah hm... while writing the commit message, I realized that there is still this special endpoint for singlefiledata.

As you mention here, this should be merged with the /nodes endpoint (we don't want to have a separate endpoint for every node type that requires a file).

@NinadBhat Can you please take care of this?
After this, I think this PR is ready to merge.

@ltalirz I thought we had decided to have two separate endpoints one for normal node and one for file upload. As one uses JSON data and the other uses form data.

thanks for the reminder @NinadBhat . After syncing with @chrisjsewell we agree to keep your approach but to move the endpoint under /nodes/singlefile.

@ltalirz Done

ltalirz · 2021-08-19T13:56:35Z

Thanks @NinadBhat !

NinadBhat added 6 commits June 11, 2021 10:27

Initial node model

331f3e4

Some node posts

48dcd89

merge master

8fea63c

file upload

e1ed06d

silencing mypy

703d2f4

Correct tests

6a5345e

Merge branch 'master' into node

1548197

NinadBhat marked this pull request as draft June 21, 2021 08:16

NinadBhat added 5 commits July 5, 2021 11:37

Uses load_entry_point_from_full_type

fafe438

add entry_points

44c4586

add importlib_metadata for python3.7

7a6e6eb

Adds aiida.rest.post

8e272df

Add entry_point for files

5135f83

NinadBhat requested review from ltalirz and chrisjsewell July 17, 2021 00:00

NinadBhat marked this pull request as ready for review July 17, 2021 00:01

NinadBhat changed the title ~~[WIP] Node post methods~~ Node post methods Jul 17, 2021

ltalirz requested changes Jul 19, 2021

View reviewed changes

NinadBhat added 3 commits July 26, 2021 16:52

review updates

c672ea2

review update

d58f612

Add custom checks for each class

0df0584

NinadBhat added 6 commits August 5, 2021 13:29

Add more tests

33fbb2b

merge errors

04e4498

Add post from orm.Code

e4ab3ff

Add Aiida Node_post model

45e2357

Adds @with_dbenv()

b136612

Merge master

7765c13

ltalirz requested changes Aug 14, 2021

View reviewed changes

NinadBhat added 2 commits August 16, 2021 22:48

Add tests

895a2f0

change aiida-version

0f78774

ltalirz approved these changes Aug 18, 2021

View reviewed changes

ltalirz requested changes Aug 18, 2021

View reviewed changes

NinadBhat added 2 commits August 18, 2021 23:47

Change docstring

735b414

change endpoint name

92a8e16

ltalirz self-requested a review August 19, 2021 13:44

ltalirz merged commit ac6e812 into aiidateam:master Aug 19, 2021

Node post methods #25

Node post methods #25

Conversation

NinadBhat commented Jun 15, 2021

NinadBhat commented Jun 17, 2021

NinadBhat commented Jun 17, 2021

ltalirz commented Jun 17, 2021

NinadBhat commented Jun 17, 2021

chrisjsewell commented Jun 17, 2021 • edited Loading

ltalirz commented Jun 17, 2021

codecov-commenter commented Jun 18, 2021 • edited Loading

Codecov Report

NinadBhat commented Jun 22, 2021

ltalirz 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

ltalirz Aug 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Aug 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Jul 19, 2021 • edited Loading

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

ltalirz commented Jul 24, 2021

ltalirz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz 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

ltalirz commented Aug 19, 2021 • edited Loading

chrisjsewell commented Jun 17, 2021 •

edited

Loading

codecov-commenter commented Jun 18, 2021 •

edited

Loading

ltalirz Aug 14, 2021 •

edited

Loading

ltalirz Aug 14, 2021 •

edited

Loading

ltalirz Jul 19, 2021 •

edited

Loading

ltalirz Jul 19, 2021 •

edited

Loading

ltalirz commented Aug 19, 2021 •

edited

Loading