Skip to content

fix(ci): make codestyle gates realistic and green-able - #6126

Closed
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/codestyle-gate-config
Closed

fix(ci): make codestyle gates realistic and green-able#6126
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/codestyle-gate-config

Conversation

@jam-jee

@jam-jee jam-jee commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Problem

codestyle-doc-tests bundles 7 tools (black-check, flake8, pylint, docstyle, twine, sphinx, doc8) and has been red on every PR while merging anyway (non-blocking). Two tools are misconfigured for this codebase, making the check structurally unwinnable:

Tool Issue Measured
flake8-future-import (FI11) Requires from __future__ import absolute_import in every file — a Python-2 relic, meaningless on py3.10+ ~107 errors/submodule
pylint --fail-under=9.9 Gate far above actual scores; unreachable without large refactor core 8.45, train 7.24, serve 7.84, mlops 7.50
pydocstyle ~600 errors/submodule, dominated by low-value/judgment rules (D400 period, D401 imperative mood, D1xx missing docstrings) ~600 → ~45 real after ignore

Changes (config only)

  1. Remove flake8-future-import from requirements/tox/flake8_requirements.txt — eliminates the FI11 noise repo-wide. Real flake8 codes (E/W/F — including F401/F811/F841 that catch actual bugs) stay fully enforced.
  2. pylint --fail-under → per-submodule floors just below current scores (core 8.0, train 7.0, serve 7.5, mlops 7.0). pylint now blocks regressions (score can't drop) while passing today — instead of a red check everyone ignores.
  3. pydocstyle --add-ignore for the noisy/judgment rules (D400/D401/D1xx/…), so docstyle enforces only high-value checks (~45 real issues remain repo-wide, fixed separately) rather than blocking on style opinion.

Gate philosophy (what this establishes)

  • Blocking, high-value, auto-fixable: black-check, flake8 (E/W/F), twine.
  • Blocking against regressions: pylint at a realistic floor.
  • Enforces only meaningful rules: docstyle.

Companion work

Testing

  • Verified per submodule with pinned tool versions: FI11 disappears with the plugin removed; pylint passes at the new floors; docstyle drops from ~600 to ~45 (real, high-value) errors.

The codestyle-doc-tests check has been red on every PR (and merges anyway)
because two of its tools were misconfigured for this codebase:

1. flake8-future-import (FI11): enforces `from __future__ import
   absolute_import` in every file — a Python-2 relic meaningless on this
   py3.10+ codebase, producing ~100+ errors per submodule. Removed the
   plugin from requirements/tox/flake8_requirements.txt.

2. pylint --fail-under=9.9: the actual scores are core 8.45, train 7.24,
   serve 7.84, mlops 7.50 — unreachable without a large refactor. Reset to
   per-submodule floors just below current (core 8.0, train 7.0, serve 7.5,
   mlops 7.0) so pylint blocks REGRESSIONS while passing today.

3. pydocstyle: ~600 errors/submodule dominated by low-value/judgment rules
   (D400 trailing period, D401 imperative mood, D1xx missing docstrings).
   Added --add-ignore for those so docstyle enforces only high-value checks
   (leaves ~45 real issues repo-wide, fixed separately) instead of blocking
   on style opinion.

black-check and flake8 (real E/W/F codes) remain fully enforced — those are
the high-value, auto-fixable gates. Companion PRs apply the black/flake8
code fixes per submodule so those envs pass.
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 2m 57s —— View job


PR Review — codestyle gate config

  • Read PR diff
  • Check flake8 config for orphaned flake8-future-import settings
  • Check pylint/pydocstyle consistency across submodules
  • Post review

Solid, well-motivated change — the rationale for each gate is clear and the per-submodule pylint floors are a good "block regressions, pass today" design. One issue needs attention before merge, plus a couple of smaller notes.

🔴 Blocking: orphaned flake8-future-import config left behind

The PR removes flake8-future-import from flake8_requirements.txt, but that plugin is what registers the require-code option. All four submodule tox.ini files still contain it:

  • sagemaker-core/tox.ini:49require-code = True
  • sagemaker-mlops/tox.ini:50require-code = True
  • sagemaker-serve/tox.ini:50require-code = True
  • sagemaker-train/tox.ini:50require-code = True

Since flake8 5.0, an unknown option key in the config file is a hard error (it was introduced specifically to catch typos). With the plugin uninstalled, require-code is no longer a registered option, so flake8 will abort at startup with something like Error: unrecognized arguments/critical config error — instead of running. That directly defeats this PR's goal of making the flake8 env green-able.

Fix: delete the require-code = True line from all four tox.ini files in this same PR.

Please verify by running flake8 in one submodule with the plugin uninstalled (i.e. after this PR's requirements change) — if it errors on require-code, this is confirmed. I couldn't execute it in this environment.

🟡 Minor: dead FI* ignore codes

Each tox.ini still lists FI10, FI12–FI18, FI50–FI58 in the [flake8] ignore block (e.g. sagemaker-core/tox.ini:30-46). These codes are only defined by flake8-future-import, so they're now no-ops. flake8 won't error on unknown codes in ignore (unlike require-code), so this isn't blocking — but for hygiene it's worth removing them in the same cleanup so a future reader doesn't think future-import checks are still active.

🟢 Looks good

  • pylint floors — per-submodule values (core 8.0, train 7.0, serve 7.5, mlops 7.0) match the measured scores in the description and are consistently applied. Good regression guard.
  • pydocstyle --add-ignore — identical ignore list applied uniformly across all four submodules; the selected codes (D1xx missing-docstring, D400/D401 judgment rules, etc.) are reasonable to defer. Consider whether the ignore list belongs in a shared [pydocstyle] config or the .pylintrc-style shared file rather than duplicated inline in four tox.inis, so it can't drift — but that's optional.

Consistency note

The two config knobs (--add-ignore, --fail-under) are applied identically across all four submodules, which is exactly right. The only inconsistency is the leftover require-code/FI* entries above — cleaning those up completes the "remove future-import" intent.

@jam-jee jam-jee closed this Jul 29, 2026
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.

1 participant