-
Notifications
You must be signed in to change notification settings - Fork 313
feat(tracer): add trace_keys option for including sample fields in trace output #874
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(tracer): add trace_keys option for including sample fields in trace output #874
Conversation
Summary of ChangesHello @JohnGiorgi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the data processing tracer by introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a valuable feature by adding the trace_id_key option, which allows for better sample identification in trace outputs. The implementation is well-structured, and the inclusion of unit tests is commendable. However, I've identified a potential bug in data_juicer/core/tracer.py where using a reserved key name (e.g., 'processed_text') for trace_id_key could lead to incorrect trace data. I have provided a detailed comment and a code suggestion to address this issue. Overall, this is a great addition with a minor correction needed.
|
Thanks for the review @gemini-code-assist! Good catch on the potential key collision issue. I've addressed this in commit a1d7b4c by:
This ensures the trace data remains accurate even if a user mistakenly includes reserved keys in |
|
Thanks for the update, @JohnGiorgi! That's an excellent approach to handle potential key collisions and ensures that the core trace information ( |
a1d7b4c to
f8a4176
Compare
Changed 'original text' to 'original_text' in trace_mapper() to be consistent with 'processed_text'. This makes the trace output keys use a consistent snake_case naming convention. Updated tests to match the new key name.
Add a new trace_id_key configuration option that allows users to specify which sample field should be included as an identifier in trace output. When trace_id_key is set, the tracer will include the specified field's value in each trace entry, enabling users to trace processed samples back to their original input data. Changes: - Add --trace_id_key argument to config parser (default: None) - Update Tracer class to accept and use trace_id_key parameter - Update trace_mapper to include ID field when configured - Pass trace_id_key from config to Tracer in default executor - Document trace_id_key in config_all.yaml - Add unit tests for trace_id_key functionality
Make the tracer configuration more flexible by allowing users to specify multiple fields to include in trace output, not just a single ID field. Changes: - Rename trace_id_key (str) to trace_keys (List[str]) - Update Tracer to iterate over list of keys - Add test for multiple trace keys - Update documentation in config_all.yaml
Address review feedback: ensure original_text and processed_text cannot be overwritten if user specifies them in trace_keys. Also improves output readability by placing trace_keys fields first in the entry.
f8a4176 to
b9af800
Compare
HYLcool
left a comment
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.
Thanks for your contribution! This is indeed a nice feature enhancement.👍🏻
Important
This PR depends on #873 and should be merged after it.
Summary
Adds a new
trace_keysconfiguration option that allows users to specify fields from the original dataset to be included in the tracer output.Usage
Via YAML config:
Or via command line:
You will now see a new field in the tracer output,
"id", which is taken straight from the original dataset:{"id":"abc-123", "original_text":"Hello", "processed_text":"Processed hello"}Why?
My primary motivation for this feature is to include a unique identifier from the original dataset in the tracer output (e.g.
"id"). I figured I would keep the feature generic to allow users to include any field they like in the tracer output, hencetrace_keys: list[str].Changes
--trace_keysargument to config parser (type:List[str], default:[])Tracerclass to accept and usetrace_keysparametertrace_mapperto include specified fields when configuredtrace_keysfrom config toTracerin default executortrace_keysinconfig_all.yamltrace_keysfunctionality (single key, multiple keys, missing field, default behavior)