Conversation
The codestyle-doc-tests CI job runs flake8, pylint, pydocstyle, black and doc8 in each of the four submodules, and every one of them has failed since the v3 split because the lint configuration did not survive the move: - .pydocstylerc lived at the v2 repo root and was dropped, so pydocstyle ran with its defaults (~6,000 findings in sagemaker-core alone). Restored from master-v2 at the root, where every submodule's ``pydocstyle src/sagemaker`` picks it up. - The generated API surface (sagemaker/core/resources.py, 35k lines, and shapes/shapes.py, 15k lines) was never excluded from flake8, pylint or pydocstyle and dominated the violation counts. It is now excluded in all three. pylint's ignore-paths must use forward slashes and ``[.]`` because pylint rewrites backslashes to derive the Windows pattern. - The pylint env installs nothing, so cross-submodule imports of the shared ``sagemaker`` namespace package produced ~700 no-name-in-module errors. Every submodule's src/ is now on PYTHONPATH in that env. - The pylint disable list was missing a comma after W0719, which made pylint read ``W0719 W1404`` as one token and silently drop both. Also disable W0718, W1203 and R0801, which the v3 code base violates by design (documented inline), and align max-line-length with the flake8 setting of 120 since black formats at 100 but does not split long strings or comments. - sagemaker-core's ``[tool.black] exclude`` replaced black's default exclusions, so the CI black-check env scanned its own .tox tree (2,500+ third-party files). Switched to extend-exclude. - FI11 (missing ``from __future__ import absolute_import``) is a no-op on Python 3 and is now ignored rather than added to ~350 files. - Every submodule's sphinx env used ``changedir = doc`` but only sagemaker-core has a docs/ directory. Fixed the path for core; for the other three the env is a documented no-op so the shared ``tox -e sphinx,doc8`` step succeeds.
Mechanical: `black ./` in each submodule with the pinned CI version (26.3.1) and the existing line-length of 100. No code changes.
Mechanical ruff autofix for F401 (unused import), F541 (f-string without placeholders), F811 (redefinition of an unused import) and W605 (invalid escape sequence), followed by black. Re-exports in __init__.py and fixture imports in conftest.py were left alone, as were notebooks and the generated files.
Behavior-preserving lint fixes so the codestyle-doc-tests job passes for this submodule: flake8 0, pydocstyle 0, pylint 9.92 (gate 9.9). Real defects the linters surfaced, fixed minimally: - model_monitor/model_monitoring.py: run_baseline built BaseliningJob from two undefined names; four describe_processing_job calls in BaseliningJob and MonitoringExecution used an undefined processing_job_name. Now use the actual locals / self attributes. - lineage/artifact.py: List["Context"] forward reference had no import; added under TYPE_CHECKING. - utils/utils.py, training/configs.py, remote_function/job.py: duplicate module-level definitions where the later one silently won; the dead earlier copy is removed. Docstrings were added for the public modules, classes and functions pydocstyle flagged and reshaped to the D212/D205 layout the restored .pydocstylerc expects. Duplicate test classes that shadowed an earlier copy with the same name are renamed (...Part1/...Part2) so both are collected; those earlier copies were never run before and may need attention if they fail. undefined-all-variable is disabled around the __all__ lists in experiments/__init__.py and utils/__init__.py, which export lazily through PEP 562 __getattr__ that pylint cannot see.
Behavior-preserving lint fixes so the codestyle-doc-tests job passes for this submodule: flake8 0, pydocstyle 0, pylint 9.91 (gate 9.9). Real defect the linters surfaced: remote_function/invoke_function.py was a stale copy of the sagemaker-core entry point that still passed ``hmac_key=`` to StoredFunction (whose parameter is ``signing_key``) and to handle_error (which takes no key). Both would raise TypeError when the remote-function job actually ran; the unit test mocked StoredFunction so it never surfaced. The file is now identical to the core version apart from the namespace, and the test kwargs follow. Also: evaluate/execution.py imported ``datetime`` inside a method that already used the module-level ``datetime`` earlier in the same scope, which made every earlier use an unbound local; the four ``from sagemaker.train import logger`` imports go through a PEP 562 __getattr__ that simply returns sagemaker.core.utils.utils.logger, so they import it from there directly; redundant in-function re-imports, a self-assignment, an unused import and ``elif``/``else`` after ``return``/``raise`` were removed.
Behavior-preserving lint fixes so the codestyle-doc-tests job passes for
this submodule: flake8 0, pydocstyle 0, pylint 9.92 (gate 9.9).
Real defects the linters surfaced, fixed minimally:
- model_builder_utils.py: the JumpStart gated-bucket path called the
zero-argument accessor JumpStartModelsAccessor.get_jumpstart_content_bucket
with a region and would raise TypeError; it now calls the region-aware
jumpstart.utils.get_jumpstart_content_bucket already imported in the
module.
- Exceptions raised with a (format, arg) tuple instead of a formatted
message (``raise ValueError("... %s", x)``) now format the message.
Left in place with an inline NOTE for the owning team rather than
guessed at: utils/lineage_utils.py calls Artifact.create with the
legacy sagemaker.lineage keyword arguments, but Artifact now resolves to
the generated core class with a different signature; and several
``prepare_*`` helpers are annotated ``-> str`` but return None, so the
assignments of their result are suppressed as assignment-from-no-return.
The ModelBuilder classes assign most of their state in build()/deploy()
rather than __init__, so attribute-defined-outside-init is disabled on
those specific classes with a reason instead of declaring hundreds of
placeholder attributes.
Behavior-preserving lint fixes so the codestyle-doc-tests job passes for this submodule: flake8 0, pydocstyle 0, pylint 9.91 (gate 9.9). Real defects the linters surfaced, fixed minimally: - workflow/retry.py: ``(a is None) == b is None`` is a chained comparison that Python reads as ``((a is None) == b) and (b is None)`` and is always false, so RetryPolicy.to_request never rejected a policy with both or neither of max_attempts / expire_after_mins. The parenthesised form now performs the intended exclusive-or check. Every in-repo caller passes exactly one of the two. - feature_processor/lineage/_feature_processor_lineage.py: a ValueError was constructed but never raised, so the "exactly one output feature group" check was a no-op. - local/pipeline_entities.py: ``type(x) != y`` is now ``type(x) is not y`` (identical for type objects, and what E721 asks for).
Four findings from the doc8 step that runs after the lint envs in the codestyle-doc-tests job.
With the lint envs green, the codestyle-doc-tests job reached its second
command, ``tox -e sphinx,doc8``, for the first time and failed in all
four submodules before running either env:
error: option --formats not recognized
ERROR: FAIL could not package project
tox 3 (what the CodeBuild image provides) builds the project sdist with
``build_sdist(..., {"--global-option": ["--formats=gztar"]})`` whenever
a selected env installs the package. setuptools dropped support for that
option in 69.0, and ``[build-system] requires = ["setuptools>=64"]``
resolves to 84.0.0 today (verified: 68.2.2 builds, 69.5.1+ fails). The
five lint envs all set ``skip_install = true`` and were unaffected.
doc8 only reads .rst files, so it now skips the install in every
submodule. sagemaker-core's sphinx env still needs an importable package
for autodoc; it now installs the project through pip (``deps =
{toxinidir}``), the same PEP 517 path ``pip install -e .[test]`` already
uses successfully earlier in the job, instead of tox's sdist step.
The codestyle-doc-tests job runs ``pip install -e .[test]`` before tox, which writes ``src/sagemaker_<pkg>.egg-info/SOURCES.txt`` (no trailing newline) into the tree. doc8's ignore list still named the v2 package, ``src/sagemaker_utils.egg-info``, so in all four submodules the only doc8 finding in CI was D005 on that generated file. Ignore ``src/*.egg-info`` instead.
… patch Two unit-test failures introduced by the lint pass: - feature_utils._is_collection_column is annotated ``-> bool`` but returned ``Series.any()``, a numpy.bool_. The tests compared with ``== True``; changing them to ``is True`` (E712) exposed the mismatch. Wrap the result in ``bool()`` so the function honours its annotation. - test_feature_scheduler.test_to_pipeline patched ``_config_uploader.TrainingInput``, a name that module imported but never used. Removing the unused import (F401) made the patch fail with AttributeError. The patch never affected the test (the mock argument was unused), so the decorator and its parameter are removed.
…xercises The ruff F401 pass removed ``from sagemaker.serve.serverless.serverless_inference_config import ServerlessInferenceConfig`` from inside the test because the name is not used, but that import is the action under test: it is what emits the DeprecationWarning the assertions look for. Without it the test recorded zero warnings. Restored with a noqa and a comment saying why it stays.
test_hosting_container_down_windows was one of the duplicate test definitions this branch un-shadowed, so it ran for the first time in CI and failed with ``ValueError: Invalid PID``. It patched ``platform.system`` to return "Windows", but ``_HostingContainer.down`` branches on ``os.name != "nt"``, so on the Linux runner it still called ``kill_child_processes`` with a Mock pid. Patch ``os.name`` in the module under test instead, which exercises the Windows branch the test describes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available
N/A. The
codestyle-doc-testsCodeBuild check has failed on every PR since the v3 monorepo split (example: #6273).Description of changes
Makes
codestyle-doc-testspass in all four submodules. That job runstox -e flake8,pylint,docstyle,black-check,twineand thentox -e sphinx,doc8in each ofsagemaker-core,sagemaker-train,sagemaker-serveandsagemaker-mlops; before this change every submodule failed flake8, pylint, pydocstyle and black-check, and the doc step never ran.Root cause
The lint configuration did not survive the move to four submodules:
.pydocstylercwas dropped, so pydocstyle ran with its defaults (~6,000 findings in core alone).sagemaker/core/resources.py, 35k lines, andshapes/shapes.py, 15k lines) was never excluded from flake8, pylint or pydocstyle and dominated the counts.sagemakernamespace package produced ~700no-name-in-moduleerrors..pylintrcdisable list (W0719/W1404) made pylint drop both entries silently.sagemaker-core's[tool.black] excludereplaced black's default exclusions, so black-check scanned the env's own.tox/tree (2,500+ third-party files).changedir = doc, but onlysagemaker-core/docsexists.Nobody had run the linters on the v3 tree since, so a large amount of drift accumulated on top.
Commits (reviewable in order)
.pydocstylerc,.pylintrc,tox.ini×4, corepyproject.toml). The one behavioural config choice: pylintmax-line-length100 → 120 to match flake8, since black formats at 100 but does not split long strings/comments.FI11(missingfrom __future__ import absolute_import, a no-op on Python 3) is ignored rather than added to ~350 files.4–7. One commit per submodule for the findings that needed judgment. Behaviour-preserving throughout, with the exceptions listed under Real defects below.
Result (CI-equivalent commands run locally with the pinned tool versions)
sphinx-buildforsagemaker-core/docssucceeds; the other three submodules have no Sphinx project, so theirsphinxenv is now a documented no-op.Real defects the linters surfaced (behaviour changes, please review)
model_monitor/model_monitoring.py:run_baselinebuiltBaseliningJobfrom two undefined names; fourdescribe_processing_jobcalls used an undefinedprocessing_job_name. WouldNameErrorat runtime.lineage/artifact.py:List["Context"]forward reference with no import.snake_to_pascal,OutputDataConfig,_get_initial_job_state) where the later copy silently won; dead earlier copies removed.remote_function/invoke_function.py: stale copy of the core entry point still passinghmac_key=toStoredFunction(parameter issigning_key) and tohandle_error(takes no key). WouldTypeErrorwhen a remote-function job ran; the unit test mockedStoredFunctionso it never surfaced. Now identical to core modulo namespace.evaluate/execution.py: an in-methodfrom datetime import datetimemade earlier uses of the module-level name unbound locals.model_builder_utils.py: JumpStart gated-bucket path called the zero-argument accessorget_jumpstart_content_bucketwith a region (TypeError); now calls the region-awarejumpstart.utilsfunction.raise Err("... %s", x)(a tuple, not a message) now format the message.workflow/retry.py:(a is None) == b is Noneis a chained comparison that is always false, soRetryPolicy.to_requestnever rejected a policy with both or neither ofmax_attempts/expire_after_mins. Now the intended exclusive check. All in-repo callers pass exactly one._feature_processor_lineage.py: aValueErrorwas constructed but never raised.Things left for the owning teams (suppressed with inline NOTEs, not guessed at)
utils/lineage_utils.py:~175callsArtifact.createwith the legacysagemaker.lineagekeyword arguments, butArtifactnow resolves to the generated core class with a different signature.prepare_*helpers are annotated-> strbut returnNone.Revived tests
Several test files contained duplicate class/function names where the later copy shadowed the earlier one, so the earlier tests were never collected. Identical copies were deleted; differing ones were renamed (
...Part1/...Part2) so both run. These tests have not run in CI before and may need attention if the unit-test job reports failures (test_session_helper.py,test_image.py,test_jumpstart_utils.py,test_tensorboard.pyin core).Testing done
Ran each submodule's CI commands locally with the pinned versions from
requirements/tox/*(black 26.3.1, flake8 7.1.2 + flake8-future-import 0.4.7, pylint 3.0.3 / astroid 3.0.2, pydocstyle 6.1.1, doc8 1.1.2). Unit tests were not run locally (dependencies not installable in the environment used); CI will run them.Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_baseto create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.