[nabinchha]

📋 Summary

Support El Salvador (es_SV) and Vietnam (vi_VN) in the managed person sampler and persona download CLI now that both prepared NGC assets are published.

🔗 Related Issue

Closes #862

🔄 Changes

• Register both locales with NGC dataset sizes: El Salvador 0.74 GB and Vietnam 2.58 GB.
• Preserve and allow filtering on care_access, disability_status, income_band, languages_spoken (El Salvador), and household_size (Vietnam).
• Document both locales and their demographic fields in the person sampling guide.
• Consolidate all locale codes, sizes, and dataset names into one explicit inventory test.
• Compare CLI discovery and all-locale downloads against the validated registry instead of fixed counts; retain representative locales for shared mechanism tests.

🧪 Testing

• 160 focused tests passed across person generation, derived fields, sampler generation/configuration, managed dataset loading, and CLI persona/download tests. New Parquet-based regressions cover output preservation and filtering for all five fields with and without synthetic personas.
• Ruff lint and format checks passed; commit hooks passed.
• NGC CLI confirms version 0.0.1 is uploaded for both assets, containing es_SV.parquet and vi_VN.parquet respectively.
• Internal links validated across 90 Fern pages. Fern check: zero errors; redirect validation skipped because CLI authentication is unavailable.
• Full suite and live NGC asset download/sampling were not run; published Parquet schemas were not inspected.

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• External contributor issue requirement: N/A (maintainer change)
• Architecture docs: N/A (locale metadata addition)

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable new issues were found.

Summary

Adds El Salvador (es_SV) and Vietnam (vi_VN) to managed person sampling and persona downloads.

• Managed person sampling and downloads now include El Salvador and Vietnam.

No actionable new issues were found. Tests were not run during this review.

_{Reviews (3) · Last reviewed commit: "fix: preserve new persona demographic fi..." · Reviewed by Greptile}

[github-actions]

Fern preview: https://nvidia-preview-pr-1136.docs.buildwithfern.com/nemo/datadesigner

Fern previews include the docs-website version archive with PR changes synced into latest. Notebook tutorials are rendered without execution outputs in previews.

[svcnvidia-nemo-ci]

No Critical or Warning findings.

I reviewed all six changed files and traced the shared locale registry through sampler validation, managed dataset loading, CLI discovery, and download construction. Both locales use the existing registry-driven paths, and existing locale entries remain unchanged.

Validation limits: I couldn't execute tests or inspect the published Parquet assets. The added engine and download tests verify loader acceptance and command construction, not the assets' schemas or live sampling behavior.

Note: formal approval is not available for this request, so this review is posted as a comment only.

Review generated by codex. This was a full review. Request a full review with /review reset=true.

[3mei]

@nabinchha The locale addition itself looks good. One suggestion on the tests:

The sampler validation, engine loader, and download registry all read from NEMOTRON_PERSONAS_DATASET_SIZES, so they don't behave differently per locale. Adding es_SV/vi_VN to the sampler, loader, and download-command tests mostly re-checks the same code path.

Could we instead:

• Keep those mechanism tests on one or two representative locales.
• Treat test_get_by_code_all_locales (explicit code/size/dataset name) as the single inventory test where new locales get added.
• Replace the hardcoded == 13 counts with a comparison against that explicit set, so future locale additions only touch constants.py and one test.

Not blocking, we could do this as a follow-up.

[nabinchha]

@3mei Thanks for the suggestion! Addressed in 8bd840b: test_get_by_code_all_locales now holds the complete explicit inventory of locale codes, sizes, and dataset names. The CLI tests compare against the registry instead of hardcoded counts, and the sampler, loader, and download-command tests retain their representative locales. Future locale additions only need changes to constants.py and the inventory test.

All 89 focused tests and Ruff checks pass.

[andreatnvidia]

Please add the new demographic fields to the sampler's field allowlist: care_access, disability_status, income_band, and languages_spoken for es_SV, and household_size for vi_VN. These fields are currently dropped from output, and using them in select_field_values raises ValueError. This differs from existing locales: India's education_degree, Japan's area, and Brazil's race are preserved and filterable, and the UK/Germany addition explicitly extended the allowlist. Please cover the new fields with output-preservation and filtering tests.

[nabinchha]

Thanks for catching this! Fixed in 93d735b: added all four es_SV fields and vi_VN household_size to the demographic allowlist. Added regression tests using local Parquet fixtures and the real reader/sampler to verify filtering and output preservation for each field, both with and without synthetic personas. All 160 focused tests and Ruff checks pass. Sex-label normalization remains tracked separately in #1138.

[andreatnvidia]

Please add es_SV and vi_VN to the supported-locale list and the locale parameter table in fern/versions/latest/pages/concepts/person_sampling.mdx. Both locations currently include all 11 existing locales but omit these two, so the documented accepted values will be incomplete after this addition.

[nabinchha]

Fixed in 93d735b: both locales are now in the supported-locale list and locale parameter table, and the page also documents their new demographic fields. Internal links across all 90 pages pass validation. Fern check reports zero errors; its only warning is that redirect validation was skipped because the CLI is not authenticated.

[andreatnvidia]

The locale registration looks good, but field support and documentation need to be completed before merging. Existing locales expose their locale-specific demographic fields through sampled output and filtering; these additions currently leave several fields inaccessible.

The sex-label normalization issue also affects existing locales and is tracked separately in #1138.