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.

Sign up for GitHub

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

Fs insert one new #166

Open
wants to merge 16 commits into
base: client
Choose a base branch
from
Open

Fs insert one new #166

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
93d02c9
init for new fs_insert_one
sl5035 Mar 21, 2023
7929b81
insert_one test function passes
sl5035 Mar 21, 2023
1123853
bug fix for test_open and test_close
sl5035 Mar 21, 2023
1a01b94
output false
sl5035 Mar 21, 2023
6f3f475
global rc
sl5035 Mar 21, 2023
0766efa
test_insert_one now passes with invalid case
sl5035 Mar 21, 2023
068a03a
runcontrol test bug fix
sl5035 Mar 22, 2023
142e7c4
test insert one now passes with bad test case
sl5035 Mar 23, 2023
0f305dd
test insert one split into two
sl5035 Mar 23, 2023
4acd773
return error info back to the client
sl5035 Mar 23, 2023
3a97060
test_insert_one_bad with return info
sl5035 Mar 23, 2023
d43a7dc
first draft
sl5035 Mar 25, 2023
72b4dea
second draft
sl5035 Mar 28, 2023
96b7659
third draft
sl5035 Mar 29, 2023
ffd5961
fsclient logic implemented
sl5035 Apr 4, 2023
f8569b6
test connect_db skip
sl5035 Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions pydatarecognition/fsclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,11 @@ def all_documents(self, collname, copy=True):

def insert_one(self, dbname, collname, doc):
"""Inserts one document to a database/collection."""
coll = self.dbs[dbname][collname]
coll[doc["_id"]] = doc
try:
coll = self.dbs[dbname][collname]
coll[doc["_id"]] = doc
except TypeError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe don't catch the exception, let the program crash? Otherwise we have bad magic that the program is not updating the database but we don't know why. Here having it crash with the exception is a feature, not a bug.

return 'Input type should be json (dict).'

def insert_many(self, dbname, collname, docs):
"""Inserts many documents into a database/collection."""
Expand Down
17 changes: 7 additions & 10 deletions tests/test_fsclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

from pydatarecognition.fsclient import FileSystemClient
from pydatarecognition.runcontrol import connect_db
from tests.inputs.pydr_rc import pydr_rc
from tests.inputs.exemplars import EXEMPLARS

#
# def test_dump_json():
Expand Down Expand Up @@ -117,24 +119,19 @@ def test_all_documents():

test_insert_cif = [({'intensity': [], 'q': [], 'ttheta': [], 'wavelength': 0.111111, '_id': 'ts1129'},
{'intensity': [], 'q': [], 'ttheta': [], 'wavelength': 0.111111, '_id': 'ts1129'}),
('bad_case_test', 'bad_case_test')]
('bad_case_test', EXEMPLARS['calculated'][-1])]
@pytest.mark.parametrize('input, result', test_insert_cif)
def test_insert_one(make_db, rc, input, result):
client = FileSystemClient(rc)
client.open()

Copy link
Collaborator

Choose a reason for hiding this comment

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

the code below seems to be testing connect_db, which should be tested in test_connect_db and not here. It is ok to use the function here to connect your db, but don't write tests for it here.

actual = client.dbs
expected = connect_db(rc)[1]
assert actual == expected

dbname = 'local'
collname = 'calculated'

try:
client.insert_one(dbname, collname, input)
assert list(client.dbs[dbname][collname].values())[0] == result
except TypeError:
print('Input type should be json (dict).')
client.load_database(pydr_rc['databases'][0])
client.insert_one(dbname, collname, input)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check we have the right logic here. I think it should look like:

  1. take exemplar and create a filesystem database with exemplar somewhere in tempdir
  2. connect to this db
  3. insert_one into this db
  4. read the file from the tempdir file
  5. make sure it contains the original exemplar and additionally the thing that was added.

I don't see the read so I am not sure we are testing that the file was updated.

Copy link
Author

Choose a reason for hiding this comment

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

I think since we are calling the rc fixture when running the test, the make_db and rc functions in conftest.py automatically generates a filesystem database with exemplar in tempdir. So if we print(client.dbs) after client.insert_one, the database contains {'local': {'calculated': {...EXEMPLAR, INSERTED_DATA}, {'measured': {...EXEMPLAR}}}}. Also this is what I originally intended to do when I wrote assert list(client.dbs[dbname][collname].values())[-1] == result.


assert list(client.dbs[dbname][collname].values())[-1] == result
Copy link
Collaborator

Choose a reason for hiding this comment

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

per your question (which I don't see here for some reason) this may be left over from Regolith where the process is to load the entire database into memory (as client.dbs[dbname][collname]). It is then put back into the database at the end with a db.dump() or sthg like that. But I think we don't want to do that here as we will have 100,000 entries in the db. so we want insert_one to insert it all the way into the database backend. In the filesystem case this will be a text file on disc containing yml or json. in the mongo case it will be a remote mongodb and for mpcontribs it will be in mpcontribs.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, quick questions here. So it's a bad practice to load client.dbs[dbname][collname] entirely using the above.> What if we implement find_one first and use it to test insert_one? For example: assert client.find_one(inserted_cif_id) == inserted_cif?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could, but I am not convinced that it is the best way. We should presumably also test that nothing else in the file was corrupted or overwritten or sthg.

Also, as I mentioned before, we are not inserting cifs per se. I think insert_one should be able to insert any valid json. I think you agree, but by putting cif in the name it makes the test harder to read so I suggest changing cif to json in the name.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! Also another quick question, by inserting all the way into the database backend you mean overriding the yaml file directly instead of using client.load_db and client.dump_db? That means we have to change our insert_one function to directly access the yaml file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I think that is right. Regolith assumed small databases that could be stored in memory, then operations on the items could be done with do loops and list comprehensions, but with large databases we want to use the powerful database capabilities directly, so we want kind of CRUD capabilities. We may probably want the fs to be json format not yml, but otherwise, yes, I think we want to insert directly.



@pytest.mark.skip("Not written")
Expand Down