-
Notifications
You must be signed in to change notification settings - Fork 12
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
DM-39582: Decrease memory usage and load times when reading graphs #348
Conversation
This cleans up a few data structures to make them smaller in memory, and makes use of new caching infrastructure in daf_butler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
doc/changes/DM-39582.api.md
Outdated
@@ -0,0 +1 @@ | |||
Deprecated reconstituteDimensions argument from QuantumNode.from_simple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably removal.md
instead of api.md
?
runner = PersistenceContextVars() | ||
graph = runner.run(self.deserializer.constructGraph, nodeSet, _readBytes, universe) | ||
return graph # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not entirely understand PersistenceContextVars/context vars, would it be nice if it could work like a regular context manager, e.g.:
with PersistenceContextVars():
return self.deserializer.constructGraph(nodeSet, _readBytes, universe)
but may be it's not possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worked on that for a while, and the outcome kept coming out more awkward, and not at all nice like that. I think it is mostly because context vars are more geared toward asyncio, callback programming style.
89015ec
to
620e96b
Compare
8b2305b
to
80046aa
Compare
80046aa
to
6d683bc
Compare
6d683bc
to
628e539
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #348 +/- ##
==========================================
- Coverage 82.82% 82.75% -0.07%
==========================================
Files 66 66
Lines 7242 7237 -5
Branches 1410 1291 -119
==========================================
- Hits 5998 5989 -9
- Misses 999 1000 +1
- Partials 245 248 +3
☔ View full report in Codecov by Sentry. |
Fixing mypy annotations revealed a small bug in adjustQuantum with dataset type names, this commit also fixes that.
Checklist
doc/changes