chore(backend): move nbprocessor constants into a separate module by harshhh817 · Pull Request #909 · kubeflow/kale

harshhh817 · 2026-07-30T04:06:09Z

What this does

Moves the constants out of nbprocessor.py into a new kale/processors/constants.py, and replaces the inline magic strings with those constants.

Part of #879 — this is the "start small" step @ada333 asked for.

Why

nbprocessor.py had module-level constants at the top and a lot of repeated string literals inline: "step_names" (10x), "tags" (11x), "limits" (5x), "prev_steps" (4x), "labels" (4x), "enable_caching" (3x), "base_image" (3x), "fns_free_variables" (3x), "code" (3x), and others. A typo in any of them fails silently — a wrong dict key reads as "not set", and a wrong getattr name returns None — rather than raising.

How it's grouped

Rather than one flat list, the new module groups by what each string actually is, since they aren't interchangeable:

Notebook JSON keys — read from the .ipynb structure (KALE_NB_METADATA_KEY, CELL_METADATA_TAGS, CELL_TYPE_CODE).
Cell tag language — the user-facing tag regexes and the two tag lists.
Parsed-tag dictionary keys — STEP_NAMES, PREV_STEPS, LIMITS, LABELS, ANNOTATIONS, BASE_IMAGE, ENABLE_CACHING. These are the contract between parse_cell_metadata (which produces the dict) and parse_notebook (which consumes it), so both sides now reference the same names.
Dynamically accessed Step attributes — FNS_FREE_VARIABLES, PARAMETERS, looked up via getattr/hasattr.
Templates and type maps — METRICS_TEMPLATE, KFP_ARTIFACT_TYPE_MAP, plus DEFAULT_VARIABLE_TYPE / DEFAULT_ARTIFACT_TYPE for the "str" / "Artifact" fallbacks.

Testing

pytest kale/tests/unit_tests → 244 passed
ruff check kale and ruff format --check kale → clean
Verified as a pure move: I compared every constant defined on main against the new module programmatically — all 21 are identical in value. No behaviour change.

Two judgment calls, happy to change either

_TAGS_LANGUAGE → TAGS_LANGUAGE (and _STEPS_DEFAULTS_LANGUAGE → STEPS_DEFAULTS_LANGUAGE). The leading underscore marked them module-private; now that they're imported across modules that's no longer accurate. Say the word if you'd rather keep the underscore.
Two .format() calls became f-strings. Not gratuitous: with the quoted dict keys replaced by constants, ruff's UP032 became applicable and ruff check fails without it. They're the only non-mechanical lines in the diff.

Follow-ups from #879 (extracting the tag parsing, dependency analysis and metrics into their own modules) are deliberately left out of this PR.

google-oss-prow · 2026-07-30T04:06:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ederign for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ada333

hey @harshhh817 looking mostly good, I have few comments

ada333 · 2026-08-03T10:18:08Z

+
+
+# Separator between a tag's parts, e.g. `limit:nvidia.com/gpu:2`.
+TAG_SEPARATOR = ":"


not used anywhere

ada333 · 2026-08-03T10:19:51Z

            if detected and (
-                ("tags" not in c.metadata or len(c.metadata["tags"]) == 0)
-                or all(not any(re.match(tag, t) for t in c.metadata["tags"]) for tag in language)
+                ("tags" not in c.metadata or len(c.metadata[CELL_METADATA_TAGS]) == 0)


here use the tags constant

ada333 · 2026-08-03T10:20:38Z

+# --------------------------------------------------------------------------
+
+FNS_FREE_VARIABLES = "fns_free_variables"
+PARAMETERS = "parameters"


not used anywhere

harshhh817 · 2026-08-04T03:49:02Z

Thanks @ada333 — all three addressed in ddb8fdb.

tags at nbprocessor.py:619 — missed literal, now uses CELL_METADATA_TAGS. There are no "tags" literals left in the file.
TAG_SEPARATOR / PARAMETERS unused — you're right, I defined both without applying them. Rather than delete them I wired them up, since both had real call sites: TAG_SEPARATOR at the four tag split/join spots, and PARAMETERS at its getattr/hasattr sites (matching how FNS_FREE_VARIABLES is used). Happy to drop them instead if you'd rather keep this PR to a pure move.

Suite still passes (268) and ruff is clean.

Per ada333's review: - Derive SECRET_TAG in nbprocessor.py from the same validator regexes applied to StepConfig.secrets (K8sSecretNameValidator/K8sSecretKeyValidator/ EnvVarNameValidator), instead of duplicating the patterns inline. Also extract the 'secret_name'/'secret_key' dict keys into shared constants in validators.py, reused by both call sites in nbprocessor.py. - SecretsSection.tsx: a row is now only written to the notebook's tags once every field (secret name, secret key, env var name) is present and matches its expected format, instead of just checking envName. The same check now also disables "Add Secret" until the current row is complete, so users can't pile up empty or malformed rows. Per harshhh817's review: - K8sSecretNameValidator now validates Secret names as DNS subdomains (dot-separated labels), matching real Kubernetes Secret name rules, instead of inheriting K8sNameValidator's stricter single-label rule. Deriving SECRET_TAG from the validator means the tag regex picks up this fix for free. - Added a template comment noting that secret_name/secret_key/env_name land inside string literals in generated code, so the validators are what keep quote characters out of them. The pipeline-level steps_defaults path bypassing field validators (harshhh817's point 2) and the kale/processors/constants.py move in kubeflow#909 (heads-up, not a review point) are pre-existing/out-of-scope for this PR. Signed-off-by: Pedro Sbaraini Cordeiro <pedro.sbarainicordeiro@gmail.com>

harshhh817 · 2026-08-18T21:26:41Z

@ada333 gentle nudge on this one whenever you have a moment — the three points from your review are addressed in ddb8fdb (the missed tags literal, plus TAG_SEPARATOR and PARAMETERS now applied at their call sites rather than sitting unused). CI is green and it's rebased on current main.

No rush — mainly flagging it because the follow-up extractions from #879 build on this one, so it's the piece gating the rest of that series.

nbprocessor.py mixed module-level constants with a large number of inline magic strings ("enable_caching", "generate_html_report", "step_names", "tags", ...). A wrong dict key reads as "not set" and a wrong getattr name returns None, so a typo fails silently. Move the existing constants and the repeated string literals into kale/processors/constants.py, grouped by what each string is: notebook JSON keys, the cell tag language, the parsed-tag dictionary keys shared between parse_cell_metadata and parse_notebook, dynamically accessed Step attributes, and templates/type maps. Pure move: every constant keeps its value. Part of kubeflow#879. Signed-off-by: Harsh Gupta <harshgupta93198@gmail.com>

…e tags constant - use CELL_METADATA_TAGS for the remaining 'tags' literal - apply TAG_SEPARATOR at the four tag split/join sites - apply PARAMETERS at its getattr/hasattr sites Signed-off-by: Harsh Gupta <harshgupta93198@gmail.com>

harshhh817 · 2026-08-27T12:45:28Z

Rebased onto current main to clear the conflict with #867 (multi-notebook composition). NOTEBOOK_TAG is folded into constants.py and added to TAGS_LANGUAGE in the same position it holds upstream (right after STEP_TAG).

Verification: all 25 constants compare byte-identical to upstream/main (checked programmatically, including the TAGS_LANGUAGE ordering), 321 tests pass, ruff check and ruff format --check clean. Still zero behaviour change.

Two judgement calls I'd rather flag than assume:

Scope. feat: add notebook: cell type for multi-notebook composition. #867 introduced "notebook_names" (4 uses) and "notebook_path" (3 uses) — the same class of literal this PR extracts. I converted them too, since leaving them behind would make the file half-converted. Happy to drop that and keep the diff to what existed when the PR opened — which would you prefer?
notebook_path spans two namespaces. It is both a cell-metadata key and a parsed-tag key, so the line reads parsed_tags[NOTEBOOK_PATH] = metadata.get(NOTEBOOK_PATH). I used one constant with a comment noting it serves both, rather than two constants with identical values. Easy to split if you'd rather keep the groups strictly separate.

ada333

lgtm!

google-oss-prow Bot requested a review from ederign July 30, 2026 04:06

google-oss-prow Bot requested a review from StefanoFioravanzo July 30, 2026 04:06

harshhh817 mentioned this pull request Jul 30, 2026

chore: Refactor nbprocessor.py #879

Open

google-oss-prow Bot added the size/L label Jul 30, 2026

harshhh817 changed the title ~~refactor(backend): move nbprocessor constants into a separate module~~ chore(backend): move nbprocessor constants into a separate module Aug 2, 2026

harshhh817 force-pushed the refactor-nbprocessor-constants branch from 5a788b4 to 50a7658 Compare August 3, 2026 05:51

ada333 suggested changes Aug 3, 2026

View reviewed changes

google-oss-prow Bot assigned ada333 Aug 3, 2026

This was referenced Aug 4, 2026

feat: add Kubernetes Secrets as environment variables to pipeline steps #891

Open

feat: Add a way to mount PVCs into Pipeline #907

Merged

Bhavd33p mentioned this pull request Aug 12, 2026

chore: share constants between backend and frontend via JSON #935

Open

5 tasks

Harsh Gupta added 2 commits August 27, 2026 14:15

harshhh817 force-pushed the refactor-nbprocessor-constants branch from ddb8fdb to dec563b Compare August 27, 2026 12:45

ada333 approved these changes Aug 28, 2026

View reviewed changes

google-oss-prow Bot added the lgtm label Aug 28, 2026

Navigation Menu

Uh oh!

chore(backend): move nbprocessor constants into a separate module - #909

chore(backend): move nbprocessor constants into a separate module#909
harshhh817 wants to merge 2 commits into
kubeflow:mainfrom
harshhh817:refactor-nbprocessor-constants

harshhh817 commented Jul 30, 2026

Uh oh!

google-oss-prow Bot commented Jul 30, 2026

Uh oh!

ada333 left a comment

Uh oh!

ada333 Aug 3, 2026

Uh oh!

ada333 Aug 3, 2026

Uh oh!

ada333 Aug 3, 2026

Uh oh!

harshhh817 commented Aug 4, 2026

Uh oh!

harshhh817 commented Aug 18, 2026

Uh oh!

harshhh817 commented Aug 27, 2026

Uh oh!

ada333 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		# Separator between a tag's parts, e.g. `limit:nvidia.com/gpu:2`.
		TAG_SEPARATOR = ":"

Uh oh!

Conversation

harshhh817 commented Jul 30, 2026

What this does

Why

How it's grouped

Testing

Two judgment calls, happy to change either

Uh oh!

google-oss-prow Bot commented Jul 30, 2026

Uh oh!

ada333 left a comment

Choose a reason for hiding this comment

Uh oh!

ada333 Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

ada333 Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

ada333 Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

harshhh817 commented Aug 4, 2026

Uh oh!

harshhh817 commented Aug 18, 2026

Uh oh!

harshhh817 commented Aug 27, 2026

Uh oh!

ada333 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants