Repository navigation
Expose translation control knobs - #2038
Conversation
Signed-off-by: rkalani <rkalani@nvidia.com>
|
/ok to test c03e067 |
Greptile SummaryThis PR exposes translation control knobs —
Confidence Score: 5/5Safe to merge — the changes are additive control-knob exposure with no modifications to existing defaults or core translation logic. All new fields default to the same values previously hard-coded, so existing callers are unaffected. The empty-batch fast-paths in SegmentationStage, ReassemblyStage, and RestoreSkippedRowsStage are well-guarded and covered by a new end-to-end test. FaithEvalFilter and FaithThresholdFilterStage already had their own empty-batch guards prior to this PR, so the full pipeline is consistent. No logic paths are altered for non-empty batches. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Caller
participant TS as TranslationStage
participant Skip as SkipExistingTranslationsStage
participant Seg as SegmentationStage
participant Tr as SegmentTranslationStage
participant FE as FaithEvalFilter
participant Re as ReassemblyStage
participant Res as RestoreSkippedRowsStage
C->>TS: process(batch)
TS->>Skip: process(batch) [if skip_translated]
Note over Skip: Stash already-translated rows in metadata, warn if all rows skipped
Skip-->>TS: remaining_df (may be empty)
TS->>Seg: process(batch)
Note over Seg: Empty-batch guard returns typed _seg_* columns
Seg-->>TS: segmented rows (or empty)
TS->>Tr: process(batch)
Note over Tr: New knobs: prompt_path, health_check, dry_run, max_concurrent_requests
Tr-->>TS: translated segments (or empty)
TS->>FE: process(batch) [if enable_faith_eval]
Note over FE: New knobs: faith_prompt_path, faith_generation_config, faith_max_concurrent_requests
FE-->>TS: scored segments (or empty)
TS->>Re: process(batch)
Note over Re: Empty-batch guard applies _OUTPUT_COLUMN_DTYPES for float64/bool columns
Re-->>TS: reassembled docs (or typed empty df)
TS->>Res: process(batch) [if skip_translated]
Note over Res: df.empty uses skipped_df.reset_index() instead of pd.concat
Res-->>TS: merged result
TS-->>C: DocumentBatch
Reviews (3): Last reviewed commit: "Merge branch 'main' into rkalani/transla..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| prompt_file = self.prompt_path or "translate.yaml" | ||
| self._system_prompt, self._user_template = load_prompt_template(prompt_file) |
There was a problem hiding this comment.
prompt_path = "" silently falls back to the default
self.prompt_path or "translate.yaml" treats an empty string as falsy, so a caller who accidentally passes prompt_path="" gets the packaged prompt with no warning. The type annotation (str | None) implies the only "no-op" value is None, so the guard should be explicit. The same pattern appears in FaithEvalFilter.setup() at the corresponding line.
| """ | ||
| if not self._initialized: | ||
| self._system_prompt, self._user_template = load_prompt_template("faith_eval.yaml") | ||
| prompt_file = self.prompt_path or "faith_eval.yaml" |
There was a problem hiding this comment.
Empty string will go to "faith_eval.yaml".
| logger.info("ReassemblyStage: no translated segment rows to reassemble") | ||
| base_cols = [col for col in df.columns if col not in _INTERNAL_COLUMNS] | ||
| out_df = df.loc[:, base_cols].copy() | ||
| for col in self.outputs()[1]: |
There was a problem hiding this comment.
is it a bit fragile that it requires outputs() returning a tuple where index [1] contains the output column names
There was a problem hiding this comment.
Agree it looks a bit fragile but it follows the convention that should be used by text stages with DocumentBatch everywhere.
|
/ok to test 4fb2a98 |
This branch was previously deployed
3 participants
Description
Exposes knobs to customize the translation workflow for downstream repos.
Checklist