Network representation optimizations for JSON encoding #217
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 81.23% 82.10% +0.86%
==========================================
Files 22 23 +1
Lines 2873 2939 +66
==========================================
+ Hits 2334 2413 +79
+ Misses 539 526 -13 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looking fantastic @ianmkenney! This is a huge improvement!
Can you also add a unit test (not integration test) for each of the methods this PR adds in alchemiscale.utils? We want to be sure in a fairly isolated way that any GufeTokenizables these methods handle are working as expected via these unit tests.
alchemiscale/interface/api.py
Outdated
| @@ -14,6 +14,7 @@ | |||
| from gufe import AlchemicalNetwork, ChemicalSystem, Transformation | |||
| from gufe.protocols import ProtocolDAGResult | |||
| from gufe.tokenization import GufeTokenizable, JSON_HANDLER | |||
| import networkx as nx | |||
There was a problem hiding this comment.
I don't think we use this import in this module.
alchemiscale/interface/api.py
Outdated
| @@ -317,12 +317,10 @@ def get_transformation( | |||
| validate_scopes(sk.scope, token) | |||
|
|
|||
| transformation = n4js.get_gufe(scoped_key=sk) | |||
| return gufe_to_json(transformation) | |||
| return GufeJSONResponse(content=transformation) | |||
There was a problem hiding this comment.
My OCD: remove content= for consistency elsewhere. 😉
alchemiscale/interface/client.py
Outdated
| @@ -17,6 +17,7 @@ | |||
| from gufe import AlchemicalNetwork, Transformation, ChemicalSystem | |||
| from gufe.tokenization import GufeTokenizable, JSON_HANDLER, GufeKey | |||
| from gufe.protocols import ProtocolResult, ProtocolDAGResult | |||
| import networkx as nx | |||
There was a problem hiding this comment.
I don't think we use this import in this module.
There was a problem hiding this comment.
seems we do, see get_transformation_tasks
There was a problem hiding this comment.
Oh that's strange; not sure how we didn't already have this import 🤔
There was a problem hiding this comment.
we did, I added it as a repeated import because I didn't notice it 😮💨
alchemiscale/utils.py
Outdated
|
|
||
|
|
||
| def gufe_to_digraph(gufe_obj): | ||
| """Recursively construct a DiGraph from a GufeTokenizable.""" |
There was a problem hiding this comment.
Add something like:
This DiGraph encodes the dependency structure of the
GufeTokenizableon otherGufeTokenizables.
| @@ -6,6 +6,9 @@ | |||
|
|
|||
| from alchemiscale.models import ScopedKey | |||
| from alchemiscale.base.client import json_to_gufe | |||
| from alchemiscale.utils import keyed_dicts_to_gufe, gufe_to_keyed_dicts | |||
|
|
|||
| import networkx as nx | |||
There was a problem hiding this comment.
I don't think this import is used in this module.
Fixes #216
This PR implements a graph based optimization for representing gufe objects as an order dependent chain of keyed dicts, dramatically reducing memory usage (~ -97% for JSON representation size of our test networks).