[PyTorch] Guard/document single parameter feature for grouped linear#2955
[PyTorch] Guard/document single parameter feature for grouped linear#2955ksivaman wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
Signed-off-by: Kirthi Shankar Sivamani <ksivamani@nvidia.com>
|
/te-ci pytorch L0 |
Greptile SummaryThis PR guards the experimental Confidence Score: 5/5Safe to merge; the gating logic is correct, warnings are informative, and CI coverage of the new path is added. Only P2 findings (minor warning-message wording). No logic bugs, no data-corruption risk, no security concerns. The function correctly short-circuits when neither flag is set, resolves at construction time, and the stacklevel is accurate for both direct-instantiation call sites. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["GroupedLinear.__init__(single_grouped_weight, single_grouped_bias)"]
A --> B["resolve_grouped_linear_single_param_flags()"]
B --> C{Either flag True?}
C -- No --> D["Return flags unchanged (no warning)"]
C -- Yes --> E{NVTE_GROUPED_LINEAR_SINGLE_PARAM > 0?}
E -- No --> F["Warn: env var not enabled\nForce both flags to False"]
E -- Yes --> G["Warn: experimental feature active\nReturn flags unchanged"]
F --> H["self.single_grouped_weight = False\nself.single_grouped_bias = False"]
G --> I["self.single_grouped_weight = requested\nself.single_grouped_bias = requested"]
D --> J["self.single_grouped_weight = False\nself.single_grouped_bias = False"]
Reviews (4): Last reviewed commit: "Merge branch 'main' into single_param_be..." | Re-trigger Greptile |
| if not (single_grouped_weight or single_grouped_bias): | ||
| return single_grouped_weight, single_grouped_bias | ||
|
|
||
| env_enabled = int(os.environ.get("NVTE_GROUPED_LINEAR_SINGLE_PARAM", "0")) > 0 |
There was a problem hiding this comment.
Non-integer env var value will raise
ValueError
int(os.environ.get("NVTE_GROUPED_LINEAR_SINGLE_PARAM", "0")) will throw an uncaught ValueError if the variable is set to a non-numeric string (e.g. "true", "yes"). Wrapping in a try/except would give a cleaner error message. This is consistent with other similar env-var checks in the file, so this is a pre-existing pattern rather than a new regression — flagging for awareness.
| env_enabled = int(os.environ.get("NVTE_GROUPED_LINEAR_SINGLE_PARAM", "0")) > 0 | |
| try: | |
| env_enabled = int(os.environ.get("NVTE_GROUPED_LINEAR_SINGLE_PARAM", "0")) > 0 | |
| except ValueError: | |
| env_enabled = False |
timmoon10
left a comment
There was a problem hiding this comment.
This logic breaks backward compatibility since we can no longer rely on the module kwargs to configure single grouped params. I guess we've already been treating single grouped params as an experimental feature. We should keep this instability in mind whenever we use this feature externally, e.g. in Mcore.
| single_grouped_weight: bool, | ||
| single_grouped_bias: bool, |
There was a problem hiding this comment.
While we are breaking backward compatibility, we might consider consolidating these options together. Do we really want to take on the burden of supporting the case with a single grouped weight and discrete bias, or discrete weights and single grouped bias?
| if not (single_grouped_weight or single_grouped_bias): | ||
| return single_grouped_weight, single_grouped_bias | ||
|
|
||
| env_enabled = int(os.environ.get("NVTE_GROUPED_LINEAR_SINGLE_PARAM", "0")) > 0 |
There was a problem hiding this comment.
If we only respect the kwargs if an envvar is set, it doesn't really make sense to keep the envvars rather than just checking the envvar. I guess we're half-heartedly maintaining/preparing the stable API for this feature.
|
/te-ci pytorch |
Signed-off-by: Kirthi Shankar Sivamani <ksivamani@nvidia.com>
|
/te-ci pytorch |
Description
Guard/document single parameter feature for grouped linear.
Type of change
Changes
Checklist: