Skip to content

gen-fields: use explicit legacy field source#127

Open
nccurry wants to merge 4 commits into
mainfrom
ncurry/legacy-field-source
Open

gen-fields: use explicit legacy field source#127
nccurry wants to merge 4 commits into
mainfrom
ncurry/legacy-field-source

Conversation

@nccurry

@nccurry nccurry commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Move curated lowercase legacy field names into pkg/dcgm/legacy_fields.csv.
  • Add --legacy-fields support to cmd/gen-fields and wire go:generate to use it.
  • Keep deprecated DCGM_FI_* aliases derived from dcgm_fields.h.

Motivation

This separates curated compatibility names from generated header-derived aliases. The generator no longer has to infer the curated lowercase names from generated output or header/comment patterns; they now live in an explicit source file that can be reviewed directly.

Testing

  • go test ./cmd/gen-fields
  • make check-generate
  • git diff --check

@nccurry nccurry requested a review from rvatkar May 28, 2026 15:16
Signed-off-by: Nick Curry <ncurry@nvidia.com>
@nccurry nccurry force-pushed the ncurry/legacy-field-source branch from e2bd544 to ad55ab1 Compare May 28, 2026 15:24
Comment thread cmd/gen-fields/main.go Outdated
legacyFields = make(map[string]int)
} else {
fmt.Fprintf(os.Stderr, "Error extracting legacy fields: %v\n", err)
legacyFields := make(map[string]int)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid successful generation with an empty curated legacy map when --legacy-fields is omitted? Verified that the old two-argument direct invocation still succeeds, but the generated legacyDCGMFields loses all lowercase dcgm_ entries. Since GetFieldID relies on those for compatibility, making the flag required or defaulting/falling back to pkg/dcgm/legacy_fields.csv would make this safer.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. I fixed it by making the no-flag path load legacy_fields.csv from the output file directory instead of proceeding with an empty curated map, so the old two-argument invocation preserves the lowercase dcgm_ compatibility names. The generator now fails loudly if that fallback CSV is missing, and I added regression coverage for the default fallback, explicit --legacy-fields, missing CSV, and help behavior. Leaving this open for you to verify and resolve.

nccurry added 3 commits June 12, 2026 10:49
Signed-off-by: Nick Curry <ncurry@nvidia.com>
Signed-off-by: Nick Curry <ncurry@nvidia.com>
Signed-off-by: Nick Curry <ncurry@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants