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

Jump to bottom

WIP: Initial models for Mongoengine (ME) classes #78

Merged
merged 44 commits into from
Dec 7, 2018
Merged

WIP: Initial models for Mongoengine (ME) classes #78

merged 44 commits into from
Dec 7, 2018

Conversation

doaa-altarawy
Copy link
Contributor

@doaa-altarawy doaa-altarawy commented Oct 25, 2018 •
edited
Loading

Description

Implementing issue #76

Todos

  • Replace Mongoengine_socket methods to use ME classes
  • Track references of any table names in the code, and only manipulate the DB through the socket methods
  • Ability to run Fractal-server completely using ME
  • Integration tests

Status

  • Ready to go

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 2 alerts when merging 13c23f5 into 03177f2 - view on LGTM.com

new alerts:

  • 2 for Unused import

Comment posted by LGTM.com

dgasmith
dgasmith reviewed Oct 26, 2018
View reviewed changes
Copy link
Contributor

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

Cool! Excited to see where this goes. Should we schedule a hackathon soon to get this in?

qcfractal/storage_sockets/models.py

def create_hash(self):
""" TODO: create a special hash before saving"""
return ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to get the full JSON document here? Should simple to do so if so.

Also should we call this Molecule as there is a Molecule class in interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a to_json() method. We always want to work with the class itself unless we will send it over the REST API.

For the molecule naming, I think there should be no issue in the future. Portal won't shouldn't import or use the storage package at all. Levi's classes should be in another package used by Portal and not by Fractal.

qcfractal/storage_sockets/models.py
driver = db.StringField(required=True) # example "gradient"
method = db.StringField(required=True) # example "uff"
basis = db.StringField()
molecule = db.ReferenceField(Molecule, required=True) # or LazyReferenceField if only ID is needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

Only ID is usually needed as we typically only pull data from return_result or properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which field are you referring to?

By referencing the class we mean it's going to be pulled and used. If we use class_id only, then it means we will rarely use it or pull it.
So

qcfractal/tests/test_mongoengine_socket.py Show resolved Hide resolved
qcfractal/tests/test_mongoengine_socket.py
assert len(result_list) == 2

# get unique by hash and formula
one_mol = Molecule.objects(molecule_hash=water_mol.molecule_hash,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can do the following in some cases as well:

query = {"molecule_hash": water_mol.molecule_hash, "molecular_formula":water_mol.molecular_formula}
.query(**query).only(molecule_hash=True).limit(5)

The attributed chaining feels awkward at the moment compared to PyMongo-- is this something that you become more accustomed to? Is there a way to do specify this in one instead of chaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chaining is the typical way, similar to Pandas (and Dash as you have mentioned).

I'll post example of how to do the query. The key here is to stop using the JSON style, lots of quotes and nested brackets :)

@dgasmith dgasmith added Enhancement Project enhancement discussion Database Related to the database storage layer Feature Adds a new feature to the project labels Oct 26, 2018
@dgasmith dgasmith added this to the v0.3.0b milestone Oct 26, 2018
@dgasmith dgasmith mentioned this pull request Oct 29, 2018
Closed
@QCArchiveBot
Copy link
Collaborator

This pull request introduces 2 alerts when merging b03d31b into f26a240 - view on LGTM.com

new alerts:

  • 2 for Unused import

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 2 alerts when merging 8a2c02b into f26a240 - view on LGTM.com

new alerts:

  • 2 for Unused import

Comment posted by LGTM.com

@dgasmith dgasmith mentioned this pull request Oct 30, 2018
Closed
@QCArchiveBot
Copy link
Collaborator

This pull request introduces 2 alerts when merging 4f3c2a7 into d5a1276 - view on LGTM.com

new alerts:

  • 2 for Unused import

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 6 alerts when merging 5116d9d into 424c633 - view on LGTM.com

new alerts:

  • 6 for Unused import

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 2 alerts when merging af9c421 into 3722afe - view on LGTM.com

new alerts:

  • 2 for Unused import

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 2 alerts when merging b0c68d1 into 3722afe - view on LGTM.com

new alerts:

  • 2 for Unused import

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 3 alerts when merging a98e8a5 into 3722afe - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Wrong name for an argument in a call

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 3 alerts when merging 5592c34 into 3722afe - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Wrong name for an argument in a call

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 3 alerts when merging 1a32177 into 3722afe - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Wrong name for an argument in a call

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 3 alerts when merging 6b1a570 into 3722afe - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Wrong name for an argument in a call

Comment posted by LGTM.com

@QCArchiveBot
Copy link
Collaborator

This pull request introduces 3 alerts when merging 13e4bb0 into 3722afe - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Wrong name for an argument in a call

Comment posted by LGTM.com

@dgasmith
Copy link
Contributor

dgasmith commented Dec 6, 2018

Certainly more work to do here, but I think everything looks good! @doaa-altarawy ready to merge?

@dgasmith dgasmith merged commit 86642a0 into MolSSI:master Dec 7, 2018
@Lnaden
Copy link
Collaborator

Lnaden commented Dec 7, 2018

Woo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgasmith dgasmith dgasmith approved these changes

Assignees
No one assigned
Labels
Database Related to the database storage layer Enhancement Project enhancement discussion Feature Adds a new feature to the project
Projects
None yet
Milestone
v0.4.0b
Development

Successfully merging this pull request may close these issues.
4 participants
@doaa-altarawy @QCArchiveBot @dgasmith @Lnaden