-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(ingestion): Adds support for memory profiling #8856
feat(ingestion): Adds support for memory profiling #8856
Conversation
metadata-ingestion/src/datahub/ingestion/run/pipeline_config.py
Outdated
Show resolved
Hide resolved
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.
a few final things, but would be good to get this merged!
Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
3f1c93f
to
4950e08
Compare
import memray | ||
|
||
with memray.Tracker( | ||
f"{self.config.flags.generate_memory_profiles}/{self.config.run_id}.bin" | ||
): |
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.
don't think this works - if generate_memory_profiles
is not set, then we don't do anything
btw you can use contextlib.ExitStack or NullContext to conditionally enter a with
clause
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.
This was an oversight on my part. Already fixed.
@pedro93 code looks good, something about the markdown file is causing issues though |
5b70849
to
b4cf270
Compare
Adds memray as opt-in memory profiler for ingestion processes.
Tested locally that it works.
Recipe:
Generates a binary flamegraph file such as
file-None-file-2023_09_18-21_38_43.bin
that can then be previewed as an HTML after running$ memray flamegraph file-None-file-2023_09_18-21_38_43.bin
locally.Sample output:
Checklist