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

support bulk_update operations involving MolField #26

Merged
merged 5 commits into from
Jul 7, 2022

Conversation

rvianello
Copy link
Contributor

This PR should fix #24

@rvianello rvianello mentioned this pull request Jul 2, 2022
@MichelML
Copy link
Contributor

MichelML commented Jul 5, 2022

@rvianello thanks for this.

I do think it will solve the bulk_update problem, so it's worth merging this for sure.

I'm not sure this will solve the problem with different version of rdkit between web app<->db as I think the problem relies in trying to pickle (with mol_from_pkl) a mol object generated from a newer version of rdkit (web app side) from the db side with an older version of rdkit. I might be wrong, but that's a seperate problem nonetheless - and I'll test once this is merged and released in a newer version of django-rdkit.

From what I understand of the PR, insertion of molecules of newer version of rdkit will work with the new implementation, but if a substructure search is later performed db side on these molecules, it likely won't work because of mol_from_pkl being used at this stage. Even that would be a win however as molecules from newer version can at least be inserted in the database.

@rvianello
Copy link
Contributor Author

@MichelML thank you for the review and feedback. This PR is not addressing the binary serialization compatibility issues that may occur when different versions of RDKit are used on the client and database server. The bulk update was failing because a Func object was passed directly to the psycopg database driver, and this triggered a client-side error. When a newer release of RDKit is used on the client side, mol_from_pkl may fail on the server side during the execution of the query (and I think this may also not happen systematically, but only when the submitted binary molecule includes features that are not yet supported by the RDKit release on the server). One way to alleviate this compatibility issue I think would consist in migrating MolField to use smiles serialization instead of binary (I'm actuallly not sure about any other solutions), and possibly make this behavior configurable in the django settings. I'll try to explore this possibility in a next PR.

@rvianello rvianello merged commit ce6bc98 into rdkit:master Jul 7, 2022
@rvianello rvianello deleted the issue/24 branch July 7, 2022 08:49
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.

Using Mol Field
2 participants