From ebcee6b536404bdb31de005d372c31ba790b849d Mon Sep 17 00:00:00 2001 From: Felipe Velasquez Date: Wed, 6 May 2026 21:21:56 -0500 Subject: [PATCH 1/5] Gate S3 boto1/boto3 default on IS_LUIGI1_DEPRECATED flag Add a private luigi.contrib._luigi1_compat module exposing IS_LUIGI1_DEPRECATED (True when `import luigi1` raises, False otherwise). luigi.contrib.s3 now binds the public S3Client and ReadableS3File aliases through this flag, so legacy environments default to the boto1 stack and modern environments default to boto3 without any per-caller change. S3ClientBoto1 / S3ClientBoto3 remain public for callers that need to pin a stack via client=. Update s3_test.py to be flag-aware (USING_BOTO1, BOTO1_RUNNABLE, S3CLIENT_INSTANTIABLE) and add TestFlagDrivenS3Dispatch plus test/contrib/luigi1_compat_test.py covering the dispatch contract. Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 76 ++++++++++ PLAN_Py39_P312_DUAL_COMPATIBILITY.md | 139 +++++++++++++++++ luigi/contrib/_luigi1_compat.py | 36 +++++ luigi/contrib/s3.py | 18 ++- test/contrib/luigi1_compat_test.py | 116 ++++++++++++++ test/contrib/s3_test.py | 216 ++++++++++++++++++++++++--- 6 files changed, 574 insertions(+), 27 deletions(-) create mode 100644 luigi/contrib/_luigi1_compat.py create mode 100644 test/contrib/luigi1_compat_test.py diff --git a/CLAUDE.md b/CLAUDE.md index e0041b1a2f..20155f0a41 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,6 +98,82 @@ python -m pytest test/ --ignore=test/contrib/mysqldb_test.py --ignore=test/visua - External `six` package (e.g. `from six.moves.urllib...`) → native `urllib.*` (Python 3.0+) - `six.PY3` checks can be removed entirely — always `True` on any supported Python 3.x +### `IS_LUIGI1_DEPRECATED` flag (boto1 ↔ boto3 dispatch) + +`luigi/contrib/_luigi1_compat.py` exposes a single private flag: + +- `IS_LUIGI1_DEPRECATED is True` → `import luigi1` raised. The legacy stack is + gone, so default to **boto3**. +- `IS_LUIGI1_DEPRECATED is False` → `luigi1` imported cleanly. Keep using the + legacy **boto1** stack for backwards compatibility. + +`luigi/contrib/s3.py` consumes the flag to bind the public aliases: + +```python +if IS_LUIGI1_DEPRECATED: + S3Client = S3ClientBoto3 + ReadableS3File = ReadableS3FileBoto3 +else: + S3Client = S3ClientBoto1 + ReadableS3File = ReadableS3FileBoto1 +``` + +Because `S3Target.__init__` (and the other `client=`-accepting classes — +`S3FlagTarget`, `S3PathTask`, `S3EmrTask`, `S3FlagTask`) fall back to +`S3Client()` when no client is injected, this single dispatch propagates +through the whole S3 surface. Callers that need to pin a specific stack +should import `S3ClientBoto1` / `S3ClientBoto3` (or `ReadableS3FileBoto1` / +`ReadableS3FileBoto3`) directly and pass the client via `client=`. + +The flag is evaluated once at module-import time. Modules that branch on it +must be re-imported (or the process restarted) if `luigi1` is added or +removed at runtime. + +The `client=` kwarg on `S3PathTask` / `S3EmrTask` / `S3FlagTask` (added in +commit `05c71137` to allow boto3 opt-in) stays. The flag covers the *default* +dispatch; the kwarg covers *explicit* injection — pinning a stack against the +env default, passing custom-credentialled or moto-mocked clients, and +supporting `RedshiftManifestTask`, which forwards `self._client` to its inner +`S3Target`. Downstream callers that previously passed `client=S3ClientBoto3()` +purely to opt into boto3 may drop that argument once `luigi1` is gone — that +cleanup is optional, not forced. + +#### Tests for the flag + +* `test/contrib/luigi1_compat_test.py` — exercises all three branches of the + flag's contract: importable (False), unimportable (True via `ImportError`), + and importable-but-raises (True via non-`ImportError`). Uses + `sys.modules` stubbing and a custom `sys.meta_path` finder; cleans up in + `tearDown` so the rest of the suite sees the real flag. +* `test/contrib/s3_test.py::TestFlagDrivenS3Dispatch` — pins the dispatch + contract: `S3Client` / `ReadableS3File` aliases match the flag, default + client injection in `S3Target` / `S3PathTask` / `S3EmrTask` / `S3FlagTask` + uses the flag-resolved class, explicit `client=` overrides the default + (same-stack always; cross-stack when both backing packages are installed), + and the aliases flip in both directions when the flag is flipped via + `importlib.reload`. +* The legacy `try: import boto / HAS_BOTO` setup at the top of `s3_test.py` + is replaced by: + - `USING_BOTO1 = S3Client is S3ClientBoto1` — does the flag point at the boto1 stack? + - `HAS_BOTO_PKG` — is the `boto` package importable? + - `BOTO1_RUNNABLE = USING_BOTO1 and HAS_BOTO_PKG` — used by the existing skip decorators (renamed from `not HAS_BOTO` to `not BOTO1_RUNNABLE`, same semantics). + - `S3CLIENT_INSTANTIABLE = HAS_BOTO_PKG if USING_BOTO1 else True` — used as a class-level `@unittest.skipUnless(...)` on `TestS3Target` and `TestS3Client`, and per-test on the dispatch tests that construct `S3Client()`. This handles the degenerate config where `luigi1` is installed (flag flips to False, so `S3Client = S3ClientBoto1`) but `boto` is missing — `S3ClientBoto1.__init__` would raise `ModuleNotFoundError`. We skip rather than fail. + - `S3ResponseError` is bound to `boto.exception.S3ResponseError` only when `BOTO1_RUNNABLE`; otherwise to `botocore.exceptions.ClientError`. This must follow the *active* stack, not just the *availability* of `boto` — boto can be on path while the flag still selects boto3, in which case errors come from botocore. + +#### Scenario matrix (`uv run --no-project --with-editable . ...`) + +| # | Python | Stack | luigi1 | Result | +|---|--------|-------|--------|--------| +| 1 | 3.12 | boto + boto3 + moto1 | no | **install fails** — `boto@2.49.0+affirm0` is not py3.12-compatible (`boto.vendored.six.moves` ModuleNotFoundError during build) | +| 2 | 3.12 | boto3 + moto5 | no | 54 pass, 13 skip | +| 3 | 3.9 | boto + boto3 + moto1 | no | 55 pass, 12 skip (cross-stack override runs because boto1 is instantiable) | +| 4 | 3.9 | boto3 + moto1 | no | 55 pass, 12 skip — but `moto==1.3.14` transitively requires `boto`, so this collapses to scenario 3 | +| 5 | 3.9 | boto3 + moto5 | no | 54 pass, 13 skip | +| 6 | 3.9 | boto + boto3 + moto1 | yes | 11 pass, 56 fail — `moto==1.3.14` does not intercept boto1's HTTP calls in this combo, so requests hit real AWS and return `InvalidAccessKeyId`. **Pre-existing test infra issue**, documented by `run_tests.sh` ("s3_test.py uses boto (SigV2) which moto v4+ no longer mocks; skip on Py39"). Not caused by the flag work. | +| 7 | 3.9 | boto3 + moto5 + luigi1 | yes | 4 pass, 63 skip — degenerate config: flag selects boto1 (luigi1 is installed) but `boto` package isn't, so all S3-touching tests skip via `S3CLIENT_INSTANTIABLE`. The 4 dispatch tests that don't instantiate `S3Client` (alias identity, `_readable_file_cls` wiring, reload-based flip test) still pass. | + +The scenarios that should run cleanly (2, 3, 5) all do. The scenarios that should fail (1, 7) fail in informative ways — install error on py3.12 boto1, and graceful skip when the flag selects an uninstalled stack. Scenario 6's failures are pre-existing and dodged in normal CI by skipping the file. + ### Known Pre-existing Test Failures (not caused by Py312 changes) - `test/contrib/mysqldb_test.py` — requires MySQL connector not installed in dev env - `test/visualiser/` — requires Selenium not installed in dev env diff --git a/PLAN_Py39_P312_DUAL_COMPATIBILITY.md b/PLAN_Py39_P312_DUAL_COMPATIBILITY.md index ea2c7892a0..3634b17e84 100644 --- a/PLAN_Py39_P312_DUAL_COMPATIBILITY.md +++ b/PLAN_Py39_P312_DUAL_COMPATIBILITY.md @@ -265,6 +265,145 @@ target = S3Target("s3://bucket/key", client=S3ClientBoto3()) --- +## May 6 2026 — `IS_LUIGI1_DEPRECATED` flag gates default boto1 ↔ boto3 dispatch + +**Supersedes** the April 13 2026 decision that pinned `S3Client = S3ClientBoto1` unconditionally. + +### Why + +Affirm has two coexisting deployment shapes during the migration: + +* **Legacy** — Affirm's internal `luigi1` package is still installed alongside this fork. Existing call sites that don't pass `client=` must continue to land on the boto1 stack. +* **Modern** — `luigi1` has been removed. The same default call sites should land on the boto3 stack with no per-caller change. + +Hard-coding the default to boto1 forced every caller in the modern environment to migrate explicitly to `client=S3ClientBoto3()` before they could drop `luigi1`. We now key the default off whether `luigi1` is importable, so the cutover is environmental rather than per-call-site. + +### What + +* New private module `luigi/contrib/_luigi1_compat.py` exporting one symbol: + + ```python + try: + import luigi1 # noqa: F401 + IS_LUIGI1_DEPRECATED = False + except Exception: + IS_LUIGI1_DEPRECATED = True + ``` + + `Exception` (not just `ImportError`) is caught so an import-time error inside `luigi1` itself also flips the flag — if `luigi1` can't be loaded, treat it as gone. + +* `luigi/contrib/s3.py` now imports the flag and resolves the public aliases conditionally: + + ```python + from luigi.contrib._luigi1_compat import IS_LUIGI1_DEPRECATED + + if IS_LUIGI1_DEPRECATED: + S3Client = S3ClientBoto3 + ReadableS3File = ReadableS3FileBoto3 + else: + S3Client = S3ClientBoto1 + ReadableS3File = ReadableS3FileBoto1 + ``` + +### Surface controlled by the flag + +Because every `client=`-accepting class falls back to `S3Client()` when no client is injected, this single dispatch flows through: + +* `S3Target.__init__` → `self.fs = client or S3Client()` +* `S3FlagTarget`, `S3EmrTarget` (subclasses of `S3Target`) +* `S3PathTask`, `S3EmrTask`, `S3FlagTask` — all forward `client` to their target constructors +* `ReadableS3File` is selected indirectly via `S3Client._readable_file_cls`, which is wired correctly on both `S3ClientBoto1` and `S3ClientBoto3`, so `S3Target.open()` always picks the matching readable-file impl. + +The classes `S3ClientBoto1`, `S3ClientBoto3`, `ReadableS3FileBoto1`, `ReadableS3FileBoto3` remain public and pinnable. Callers that need a specific stack regardless of environment continue to import the concrete class and pass it via `client=`. + +### Files changed + +* **new** — `luigi/contrib/_luigi1_compat.py` +* **modified** — `luigi/contrib/s3.py` (added flag import; replaced static aliases with conditional dispatch at module bottom) +* **docs** — `CLAUDE.md`, this file + +### Out of scope (no fork to gate) + +`luigi/contrib/batch.py`, `luigi/contrib/ecs.py`, and `luigi/notifications.py` already use boto3 only — no boto1 implementation exists for them, so the flag does not apply. + +### Verification performed + +* Import smoke test in the dev venv (`luigi1` not installed) — `IS_LUIGI1_DEPRECATED == True`, `S3Client is S3ClientBoto3`, `ReadableS3File is ReadableS3FileBoto3`, and `S3Target('s3://x/y').fs` is an `S3ClientBoto3`. +* Inverse smoke test with `sys.modules['luigi1']` stubbed and `luigi.contrib.s3` reloaded — `IS_LUIGI1_DEPRECATED == False`, aliases resolve back to the boto1 stack. + +### Caveat + +The flag is evaluated once at module-import time. If `luigi1` is added or removed at runtime, modules that branch on the flag must be re-imported (or the process restarted) for the change to take effect. This matches normal Python import semantics and is acceptable for the deployment shapes above. + +### Evaluation — are the `client=` parameters on `S3PathTask` / `S3EmrTask` / `S3FlagTask` still required? + +The April 13 commit `05c71137` added a `client=None` kwarg to `S3PathTask`, `S3EmrTask`, and `S3FlagTask` so callers could opt into boto3. With the flag now driving the default, that *specific* motivation is gone — `S3PathTask(path=...)` already lands on `S3ClientBoto3` whenever `IS_LUIGI1_DEPRECATED == True`. **However, keep the parameters.** They remain useful for: + +* **Pinning a stack against the env-driven default.** Callers in a legacy env (`luigi1` still installed → flag False → default boto1) who need boto3 for a specific task, or vice versa, still need an injection point. +* **General dependency injection** — custom credentials, assumed-role clients, alternate regions, moto-mocked clients in tests. This is independent of the boto1↔boto3 dichotomy and matches the long-standing `S3Target(client=...)` pattern. +* **`RedshiftManifestTask`** (`luigi/contrib/redshift.py:518`) calls `S3Target(folder_path, client=self._client)` where `self._client` comes from the inherited `S3PathTask.__init__`. Removing the parameter would force a parallel rewrite there. + +In this repo, no call site currently passes an explicit `client=S3ClientBoto1()` / `client=S3ClientBoto3()` to those task classes, so flipping the flag introduces no in-repo redundancy. Downstream consumers (e.g., `all-the-things`) that previously added `client=S3ClientBoto3()` purely to opt into boto3 may now drop that argument once `luigi1` is gone — that cleanup is optional, not forced. + +**Conclusion:** the flag handles the *default* dispatch; the `client=` parameter handles *explicit* injection. They compose; both stay. + +### Test coverage for the flag + +* **`test/contrib/luigi1_compat_test.py`** — new file. Pins the three branches of `IS_LUIGI1_DEPRECATED`: + * `True` when `import luigi1` raises `ImportError` (real env state) + * `False` when `luigi1` is importable (stubbed via `sys.modules['luigi1'] = types.ModuleType(...)` and `importlib.reload`) + * `True` when `import luigi1` raises a non-`ImportError` exception (simulated via a custom `sys.meta_path` finder) + + The test class snapshots and restores `sys.modules['luigi1']` and `sys.meta_path` in `setUp`/`tearDown`, and re-imports `luigi.contrib._luigi1_compat` on exit so subsequent test files see the real flag. + +* **`test/contrib/s3_test.py::TestFlagDrivenS3Dispatch`** — new test class in the existing s3 test file. Pins the dispatch contract end-to-end: + * `S3Client is S3ClientBoto3` and `ReadableS3File is ReadableS3FileBoto3` when the flag is True; the boto1 equivalents when False. + * `S3ClientBoto1._readable_file_cls` and `S3ClientBoto3._readable_file_cls` are wired to the matching readable-file impl, so `S3Target.open()` resolves correctly regardless of which client was injected. + * `S3Target('s3://...')`, `S3PathTask(path=...).output()`, `S3EmrTask(path=...).output()`, and `S3FlagTask(path=...).output()` all default `fs` to an instance of the flag-resolved `S3Client`. + * Same-stack explicit `client=` override: an injected instance is preserved verbatim by `S3Target`. + * Cross-stack explicit override: same assertion against the *other* stack — runs only when both backing packages are installed (skipped otherwise). + * Aliases flip in both directions when `luigi.contrib._luigi1_compat` and `luigi.contrib.s3` are reloaded with `luigi1` stubbed in / out of `sys.modules`. Cleanup re-imports the real modules so subsequent tests aren't poisoned. + +* **`test/contrib/s3_test.py` setup change** — the legacy `try: import boto / HAS_BOTO` block is replaced by `USING_BOTO1 = S3Client is S3ClientBoto1`. Every existing `@unittest.skipIf(not HAS_BOTO, ...)` decorator is preserved verbatim as `@unittest.skipIf(not USING_BOTO1, ...)` — same skip semantics, now sourced from the flag rather than from probing `boto`. The previous `S3Client = S3ClientBoto3` rebinding (when `boto` was missing) is no longer needed because `S3Client` is already flag-resolved at import time. + +### Test results in the dev env (`luigi1` not installed → flag True → boto3 default) + +`pytest test/contrib/luigi1_compat_test.py test/contrib/s3_test.py`: **58 passed, 13 skipped**. The 13 skips break down as 12 boto1-only tests (encrypt_key, `key.Key.BufferSize`, boto-specific credential attrs) plus one cross-stack override that needs `S3ClientBoto1()` to be instantiable. All consistent with running on the boto3 stack without `boto` installed. + +### Test infra refinements after running the 7-scenario uv matrix + +Two fragility points surfaced when running the test file under scenarios that flip the flag (luigi1 installed): + +1. **Top-level `from boto.exception import S3ResponseError` blew up at collection time** when the flag selected boto1 but `boto` was not installed (scenario 7). Wrapped in `try/except` and added two new module-level predicates: + * `HAS_BOTO_PKG` — is the boto package importable? + * `BOTO1_RUNNABLE = USING_BOTO1 and HAS_BOTO_PKG` — used by all existing `@unittest.skipIf` decorators (renamed from `not HAS_BOTO`/`not USING_BOTO1`, identical semantics). + * `S3CLIENT_INSTANTIABLE = HAS_BOTO_PKG if USING_BOTO1 else True` — used as a class-level `@unittest.skipUnless(...)` on `TestS3Target` and `TestS3Client` and per-test on the instantiating dispatch tests, so the file collects and the dispatch contract still gets verified even in degenerate configs. + +2. **`S3ResponseError` binding** previously followed *boto-package-availability*, but with the flag in play that's no longer the same as *active-stack*. boto can be on path while the flag still selects boto3 (scenario 3 — boto installed, no luigi1, flag True). Now bound based on the active stack: + ```python + S3ResponseError = _Boto1ResponseError if (USING_BOTO1 and HAS_BOTO_PKG) else _Boto3ResponseError + ``` + +3. **`test_aliases_flip_when_flag_flipped`** previously simulated "flag → True" by `sys.modules.pop('luigi1')` and reloading. That fails when `luigi1` is genuinely installed because Python re-imports it from disk. Replaced with an active `sys.meta_path` finder that raises `ImportError` for `luigi1` during the reload. + +### `uv run` scenario matrix + +Each scenario runs `uv run --python --no-project --index-url https://pypi.affirm-build.com/artifactory/api/pypi/pypi/simple/ --with-editable . --with pytest --with mock python -m pytest -q test/contrib/s3_test.py --override-ini addopts=''`. + +| # | Python | Stack deps | luigi1 | Result | Notes | +|---|--------|-----------|--------|--------|-------| +| 1 | 3.12 | boto + boto3==1.33.13 + moto==1.3.14 | — | **install fails** | `boto@2.49.0+affirm0` references `boto.vendored.six.moves`, not py3.12-compatible | +| 2 | 3.12 | boto3 + moto>=5,<6 | — | 54 / 13 skip / 0 fail | modern stack, flag True, S3Client=S3ClientBoto3 | +| 3 | 3.9 | boto + boto3==1.33.13 + moto==1.3.14 | — | 55 / 12 skip / 0 fail | flag True (no luigi1), S3Client=S3ClientBoto3 even though boto is installed; cross-stack override runs | +| 4 | 3.9 | boto3==1.33.13 + moto==1.3.14 | — | 55 / 12 skip / 0 fail | moto1 transitively requires boto, so this collapses to scenario 3 | +| 5 | 3.9 | boto3 + moto>=5,<6 | — | 54 / 13 skip / 0 fail | clean modern py3.9 | +| 6 | 3.9 | boto + boto3==1.33.13 + moto==1.3.14 | luigi1==1.1.2+affirm.1.1.1 | 11 / 0 skip / 56 fail | flag False, S3Client=S3ClientBoto1 — failures are **pre-existing** moto1+boto1 mocking issues (real AWS leak), already known and dodged by `run_tests.sh --ignore=test/contrib/s3_test.py` on py3.9 | +| 7 | 3.9 | boto3 + moto>=5,<6 + luigi1 | luigi1 | 4 / 63 skip / 0 fail | degenerate: flag False (luigi1 installed) but boto missing; all S3-touching tests skip via `S3CLIENT_INSTANTIABLE`; flag dispatch contract still verified by the 4 non-instantiating tests | + +Scenarios that should run cleanly (2, 3, 5) do. Scenarios that should fail (1, 7) fail informatively. Scenario 6's failures are pre-existing test-infra issues unrelated to the flag work. + +--- + ## Migration Guide — `all-the-things` Repo The following covers every distinct usage pattern found in the `all-the-things` codebase and what, if anything, needs to change to migrate to boto3. diff --git a/luigi/contrib/_luigi1_compat.py b/luigi/contrib/_luigi1_compat.py new file mode 100644 index 0000000000..97f58df9cd --- /dev/null +++ b/luigi/contrib/_luigi1_compat.py @@ -0,0 +1,36 @@ +# -*- coding: utf-8 -*- +# +# Copyright 2024 Affirm +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +""" +Private compatibility flag used by ``luigi.contrib`` to switch behaviour +between the legacy boto1-based stack (used while the ``luigi1`` package +is still installed alongside this one) and the modern boto3-based stack +(used once ``luigi1`` has been removed from the environment). + +The single public name in this module is :data:`IS_LUIGI1_DEPRECATED`: + +* ``False`` — ``luigi1`` is importable. Callers should keep using the + legacy boto1 implementations (``S3ClientBoto1``, ``ReadableS3FileBoto1``, + ...) for backwards compatibility with the legacy stack. +* ``True`` — ``luigi1`` is no longer available (its import raised). Callers + should use the modern boto3 implementations. + +This module is intentionally private (leading underscore): it exists to +gate internal dispatch in ``luigi.contrib`` and is not part of the public +API. +""" + +try: + import luigi1 # noqa: F401 + IS_LUIGI1_DEPRECATED = False +except Exception: + # Any import-time failure (ImportError, or an error raised inside + # luigi1 itself) means the legacy stack is no longer usable. + IS_LUIGI1_DEPRECATED = True diff --git a/luigi/contrib/s3.py b/luigi/contrib/s3.py index 77fc0b285e..22582428b2 100644 --- a/luigi/contrib/s3.py +++ b/luigi/contrib/s3.py @@ -39,6 +39,7 @@ from configparser import NoSectionError from luigi import configuration +from luigi.contrib._luigi1_compat import IS_LUIGI1_DEPRECATED from luigi.format import get_default_format from luigi.parameter import OptionalParameter, Parameter from luigi.target import FileAlreadyExists, FileSystem, FileSystemException, FileSystemTarget, AtomicLocalFile, MissingParentDirectory @@ -1229,7 +1230,16 @@ def _exists(self, bucket, key): return True -# Backwards-compatible aliases — preserve existing default behaviour (boto1). -# To use boto3 explicitly, import S3ClientBoto3 directly and pass it as client=. -S3Client = S3ClientBoto1 -ReadableS3File = ReadableS3FileBoto1 +# Default S3 client / readable-file implementations. The choice is gated by +# IS_LUIGI1_DEPRECATED (see luigi/contrib/_luigi1_compat.py): while luigi1 is +# still installed alongside this package the legacy boto1 stack is used to +# preserve backwards compatibility; once luigi1 has been removed the modern +# boto3 stack takes over. Callers that need to pin a specific implementation +# should import S3ClientBoto1/S3ClientBoto3 (or the matching readable-file +# class) directly and pass it via the ``client=`` kwarg. +if IS_LUIGI1_DEPRECATED: + S3Client = S3ClientBoto3 + ReadableS3File = ReadableS3FileBoto3 +else: + S3Client = S3ClientBoto1 + ReadableS3File = ReadableS3FileBoto1 diff --git a/test/contrib/luigi1_compat_test.py b/test/contrib/luigi1_compat_test.py new file mode 100644 index 0000000000..7e67c6c6be --- /dev/null +++ b/test/contrib/luigi1_compat_test.py @@ -0,0 +1,116 @@ +# -*- coding: utf-8 -*- +# +# Copyright 2024 Affirm +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +"""Tests for ``luigi.contrib._luigi1_compat.IS_LUIGI1_DEPRECATED``. + +The flag's contract: +* ``True`` — ``import luigi1`` raised (any ``Exception``). +* ``False`` — ``luigi1`` imported cleanly. + +These tests exercise all three branches of that contract: importable, +unimportable (``ImportError``), and importable-but-raises. +""" + +import importlib +import sys +import types + +from helpers import unittest + + +COMPAT_MODULE = 'luigi.contrib._luigi1_compat' + + +def _force_reimport_compat(): + """Drop and re-import the compat module so its top-level ``import luigi1`` + re-runs against the current ``sys.modules`` / ``sys.meta_path`` state.""" + sys.modules.pop(COMPAT_MODULE, None) + return importlib.import_module(COMPAT_MODULE) + + +class _RaiseOnImportFinder: + """A ``sys.meta_path`` finder that raises a chosen exception when a + specific module name is imported. Used to simulate ``luigi1`` raising + something other than ``ImportError`` at import time.""" + + def __init__(self, target_name, exc): + self.target_name = target_name + self.exc = exc + + def find_spec(self, fullname, path, target=None): + if fullname == self.target_name: + raise self.exc + return None + + +class TestLuigi1CompatFlag(unittest.TestCase): + + def setUp(self): + # Snapshot the modules we may mutate so we can restore cleanly. + self._saved_luigi1 = sys.modules.get('luigi1', None) + self._had_luigi1 = 'luigi1' in sys.modules + self._saved_meta_path = list(sys.meta_path) + + def tearDown(self): + # Restore module + meta_path state. + if self._had_luigi1: + sys.modules['luigi1'] = self._saved_luigi1 + else: + sys.modules.pop('luigi1', None) + sys.meta_path[:] = self._saved_meta_path + # Re-import the compat module so the rest of the test suite sees the + # real flag value for this environment. + _force_reimport_compat() + + def test_flag_is_bool(self): + compat = importlib.import_module(COMPAT_MODULE) + self.assertIsInstance(compat.IS_LUIGI1_DEPRECATED, bool) + + def test_flag_true_when_luigi1_unimportable(self): + # In the dev/CI env luigi1 is not installed. Make sure we evaluate the + # flag against that clean state. + sys.modules.pop('luigi1', None) + compat = _force_reimport_compat() + self.assertTrue( + compat.IS_LUIGI1_DEPRECATED, + "expected IS_LUIGI1_DEPRECATED == True when luigi1 cannot be imported", + ) + + def test_flag_false_when_luigi1_importable(self): + # Stub luigi1 so the top-level ``import luigi1`` in the compat module + # succeeds without finding the real package. + sys.modules['luigi1'] = types.ModuleType('luigi1') + compat = _force_reimport_compat() + self.assertFalse( + compat.IS_LUIGI1_DEPRECATED, + "expected IS_LUIGI1_DEPRECATED == False when luigi1 imports cleanly", + ) + + def test_flag_true_when_luigi1_import_raises_non_importerror(self): + # The compat module catches ``Exception``, not just ``ImportError``, + # so an arbitrary import-time failure inside luigi1 should still + # flip the flag to True. + sys.modules.pop('luigi1', None) + finder = _RaiseOnImportFinder('luigi1', RuntimeError('luigi1 init blew up')) + sys.meta_path.insert(0, finder) + try: + compat = _force_reimport_compat() + finally: + # tearDown also resets sys.meta_path, but we want any reload below + # in this test to use the original list. + sys.meta_path.remove(finder) + self.assertTrue( + compat.IS_LUIGI1_DEPRECATED, + "expected IS_LUIGI1_DEPRECATED == True when import luigi1 raises a non-ImportError", + ) + + +if __name__ == '__main__': + unittest.main() diff --git a/test/contrib/s3_test.py b/test/contrib/s3_test.py index c382c756c2..78acd3d8a5 100644 --- a/test/contrib/s3_test.py +++ b/test/contrib/s3_test.py @@ -24,20 +24,64 @@ from helpers import with_config, unittest, skipOnTravis from luigi import configuration -from luigi.contrib.s3 import FileNotFoundException, InvalidDeleteException, S3Client, S3ClientBoto3, S3Target +from luigi.contrib._luigi1_compat import IS_LUIGI1_DEPRECATED +from luigi.contrib.s3 import ( + FileNotFoundException, + InvalidDeleteException, + ReadableS3File, + ReadableS3FileBoto1, + ReadableS3FileBoto3, + S3Client, + S3ClientBoto1, + S3ClientBoto3, + S3EmrTask, + S3FlagTask, + S3PathTask, + S3Target, +) from luigi.target import MissingParentDirectory +# `S3Client` is the flag-resolved alias from luigi.contrib.s3: +# IS_LUIGI1_DEPRECATED is False → S3Client is S3ClientBoto1 (legacy boto1 stack) +# IS_LUIGI1_DEPRECATED is True → S3Client is S3ClientBoto3 (modern boto3 stack) +USING_BOTO1 = S3Client is S3ClientBoto1 + +# Try to import boto1 unconditionally so the file can collect cleanly even +# in degenerate configs (e.g. luigi1 installed but boto missing). The +# resulting symbol bindings let us pick the right exception class to match +# whichever stack is actually live. try: - import boto - from boto.exception import S3ResponseError + from boto.exception import S3ResponseError as _Boto1ResponseError from boto.s3 import key - HAS_BOTO = True + HAS_BOTO_PKG = True except ImportError: - import boto3 - from botocore.exceptions import ClientError as S3ResponseError - HAS_BOTO = False - # boto1 is not available; use the boto3 client for all tests in this file - S3Client = S3ClientBoto3 + _Boto1ResponseError = None + key = None + HAS_BOTO_PKG = False + +from botocore.exceptions import ClientError as _Boto3ResponseError + +# ``S3ResponseError`` must match whatever the active stack actually raises, +# not just whichever package happens to be installed. e.g. boto can be on +# the path while the flag still selects boto3 — in that case errors come +# from botocore, not boto. +if USING_BOTO1 and HAS_BOTO_PKG: + S3ResponseError = _Boto1ResponseError +else: + S3ResponseError = _Boto3ResponseError + +# Tests that exercise boto1-only behaviour (encrypt_key kwarg, +# key.Key.BufferSize, boto-specific credential attrs) need both the flag +# pointing at boto1 AND the boto package to be importable. +BOTO1_RUNNABLE = USING_BOTO1 and HAS_BOTO_PKG + +# True when ``S3Client()`` (the flag-resolved default) can actually be +# instantiated in this env. ``S3ClientBoto1.__init__`` does ``from boto.s3.key +# import Key``, so when the flag selects boto1 but the boto package is missing, +# instantiation fails. Tests that construct ``S3Client()`` (or any class that +# transitively does, like ``S3Target('s3://...')`` with no explicit client) +# must skip in that degenerate case. +S3CLIENT_INSTANTIABLE = HAS_BOTO_PKG if USING_BOTO1 else True try: from moto import mock_s3, mock_sts @@ -54,6 +98,8 @@ AWS_SECRET_KEY = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" +@unittest.skipUnless(S3CLIENT_INSTANTIABLE, + 'flag selects boto1 stack but boto package is not installed') class TestS3Target(unittest.TestCase, FileSystemTargetTestMixin): def setUp(self): @@ -71,7 +117,7 @@ def setUp(self): self.addCleanup(self.mock_s3.stop) def _create_bucket(self, client): - if HAS_BOTO: + if USING_BOTO1: client.s3.create_bucket('mybucket') else: import boto3 @@ -96,12 +142,12 @@ def test_read_no_file(self): t = self.create_target() self.assertRaises(FileNotFoundException, t.open) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_read_no_file_sse(self): t = self.create_target(encrypt_key=True) self.assertRaises(FileNotFoundException, t.open) - @unittest.skipIf(not HAS_BOTO, 'boto Key.BufferSize not available with boto3') + @unittest.skipIf(not BOTO1_RUNNABLE, 'boto Key.BufferSize not available with boto3') def test_read_iterator_long(self): # write a file that is 5X the boto buffersize # to test line buffering @@ -134,13 +180,15 @@ def test_get_path(self): path = t.path self.assertEqual('s3://mybucket/test_file', path) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_get_path_sse(self): t = self.create_target(encrypt_key=True) path = t.path self.assertEqual('s3://mybucket/test_file', path) +@unittest.skipUnless(S3CLIENT_INSTANTIABLE, + 'flag selects boto1 stack but boto package is not installed') class TestS3Client(unittest.TestCase): def setUp(self): @@ -159,14 +207,14 @@ def setUp(self): self.addCleanup(self.mock_sts.stop) def _create_bucket(self, client=None, name='mybucket'): - if HAS_BOTO: + if USING_BOTO1: (client or S3Client(AWS_ACCESS_KEY, AWS_SECRET_KEY)).s3.create_bucket(name) else: import boto3 conn = boto3.resource('s3', region_name='us-east-1') conn.create_bucket(Bucket=name) - @unittest.skipIf(not HAS_BOTO, 'boto-specific credential attribute gs_access_key_id') + @unittest.skipIf(not BOTO1_RUNNABLE, 'boto-specific credential attribute gs_access_key_id') def test_init_with_environment_variables(self): os.environ['AWS_ACCESS_KEY_ID'] = 'foo' os.environ['AWS_SECRET_ACCESS_KEY'] = 'bar' @@ -180,14 +228,14 @@ def test_init_with_environment_variables(self): self.assertEqual(s3_client.s3.gs_access_key_id, 'foo') self.assertEqual(s3_client.s3.gs_secret_access_key, 'bar') - @unittest.skipIf(not HAS_BOTO, 'boto-specific credential attributes access_key/secret_key') + @unittest.skipIf(not BOTO1_RUNNABLE, 'boto-specific credential attributes access_key/secret_key') @with_config({'s3': {'aws_access_key_id': 'foo', 'aws_secret_access_key': 'bar'}}) def test_init_with_config(self): s3_client = S3Client() self.assertEqual(s3_client.s3.access_key, 'foo') self.assertEqual(s3_client.s3.secret_key, 'bar') - @unittest.skipIf(not HAS_BOTO, 'boto-specific STS credential attributes') + @unittest.skipIf(not BOTO1_RUNNABLE, 'boto-specific STS credential attributes') @with_config({'s3': {'aws_role_arn': 'role', 'aws_role_session_name': 'name'}}) def test_init_with_config_and_roles(self): s3_client = S3Client() @@ -200,7 +248,7 @@ def test_put(self): s3_client.put(self.tempFilePath, 's3://mybucket/putMe') self.assertTrue(s3_client.exists('s3://mybucket/putMe')) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_put_sse(self): s3_client = S3Client(AWS_ACCESS_KEY, AWS_SECRET_KEY) self._create_bucket(s3_client) @@ -213,7 +261,7 @@ def test_put_string(self): s3_client.put_string("SOMESTRING", 's3://mybucket/putString') self.assertTrue(s3_client.exists('s3://mybucket/putString')) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_put_string_sse(self): s3_client = S3Client(AWS_ACCESS_KEY, AWS_SECRET_KEY) self._create_bucket(s3_client) @@ -229,7 +277,7 @@ def test_put_multipart_multiple_parts_non_exact_fit(self): file_size = (part_size * 2) - 5000 self._run_multipart_test(part_size, file_size) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_put_multipart_multiple_parts_non_exact_fit_with_sse(self): part_size = (1024 ** 2) * 5 file_size = (part_size * 2) - 5000 @@ -243,7 +291,7 @@ def test_put_multipart_multiple_parts_exact_fit(self): file_size = part_size * 2 self._run_multipart_test(part_size, file_size) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_put_multipart_multiple_parts_exact_fit_wit_sse(self): part_size = (1024 ** 2) * 5 file_size = part_size * 2 @@ -257,7 +305,7 @@ def test_put_multipart_less_than_split_size(self): file_size = 5000 self._run_multipart_test(part_size, file_size) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_put_multipart_less_than_split_size_with_sse(self): part_size = (1024 ** 2) * 5 file_size = 5000 @@ -271,7 +319,7 @@ def test_put_multipart_empty_file(self): file_size = 0 self._run_multipart_test(part_size, file_size) - @unittest.skipIf(not HAS_BOTO, 'encrypt_key is boto-only') + @unittest.skipIf(not BOTO1_RUNNABLE, 'encrypt_key is boto-only') def test_put_multipart_empty_file_with_sse(self): part_size = (1024 ** 2) * 5 file_size = 0 @@ -531,3 +579,125 @@ def _run_multipart_test(self, part_size, file_size, **kwargs): key_size = s3_client.get_key(s3_path).size self.assertEqual(file_size, key_size) tmp_file.close() + + +class TestFlagDrivenS3Dispatch(unittest.TestCase): + """``S3Client`` / ``ReadableS3File`` and the default client injected by + ``S3Target`` / ``S3PathTask`` / ``S3EmrTask`` / ``S3FlagTask`` are all + chosen by ``IS_LUIGI1_DEPRECATED``. These tests pin that contract.""" + + def test_alias_matches_current_flag(self): + if IS_LUIGI1_DEPRECATED: + self.assertIs(S3Client, S3ClientBoto3) + self.assertIs(ReadableS3File, ReadableS3FileBoto3) + else: + self.assertIs(S3Client, S3ClientBoto1) + self.assertIs(ReadableS3File, ReadableS3FileBoto1) + + def test_readable_file_class_wired_to_each_client(self): + # Independently of which is currently aliased as the default, each + # client class must point at its matching readable-file impl so that + # S3Target.open() resolves correctly regardless of which client was + # injected. + self.assertIs(S3ClientBoto1._readable_file_cls, ReadableS3FileBoto1) + self.assertIs(S3ClientBoto3._readable_file_cls, ReadableS3FileBoto3) + + @unittest.skipUnless(S3CLIENT_INSTANTIABLE, 'flag selects boto1 but boto is not installed') + def test_default_client_injection_in_s3_target(self): + target = S3Target('s3://mybucket/key') + self.assertIsInstance(target.fs, S3Client) + + @unittest.skipUnless(S3CLIENT_INSTANTIABLE, 'flag selects boto1 but boto is not installed') + def test_default_client_injection_in_s3_path_task(self): + task = S3PathTask(path='s3://mybucket/key') + self.assertIsInstance(task.output().fs, S3Client) + + @unittest.skipUnless(S3CLIENT_INSTANTIABLE, 'flag selects boto1 but boto is not installed') + def test_default_client_injection_in_s3_emr_task(self): + # S3EmrTarget (via S3FlagTarget) requires path to end with '/' + task = S3EmrTask(path='s3://mybucket/dir/') + self.assertIsInstance(task.output().fs, S3Client) + + @unittest.skipUnless(S3CLIENT_INSTANTIABLE, 'flag selects boto1 but boto is not installed') + def test_default_client_injection_in_s3_flag_task(self): + task = S3FlagTask(path='s3://mybucket/dir/') + self.assertIsInstance(task.output().fs, S3Client) + + @unittest.skipUnless(S3CLIENT_INSTANTIABLE, 'flag selects boto1 but boto is not installed') + def test_explicit_client_overrides_flag_default(self): + # Same-stack override — always runs (when S3Client is instantiable). + # Confirms the ``client or S3Client()`` branch in S3Target.__init__ + # uses the injected instance verbatim, regardless of which stack the + # flag selected as the default. + same_stack = S3Client() + target = S3Target('s3://mybucket/key', client=same_stack) + self.assertIs(target.fs, same_stack) + + def test_explicit_client_cross_stack_override(self): + # Cross-stack override — pick the OTHER stack from the flag-resolved + # default and confirm S3Target uses the injected instance. Skipped + # when the other stack's backing package is not installed. + other_cls = S3ClientBoto1 if S3Client is S3ClientBoto3 else S3ClientBoto3 + try: + cross_stack = other_cls() + except ImportError: + self.skipTest( + '{} backing package not installed; cross-stack override path ' + 'not exercised in this env'.format(other_cls.__name__) + ) + target = S3Target('s3://mybucket/key', client=cross_stack) + self.assertIs(target.fs, cross_stack) + self.assertIsInstance(target.fs, other_cls) + + def test_aliases_flip_when_flag_flipped(self): + # Reload luigi.contrib.s3 with luigi1 forcibly importable (flag → False) + # and confirm the aliases flip to the boto1 stack; then with luigi1 + # forcibly unimportable (flag → True) and confirm they flip back to + # boto3. Both directions must be simulated actively because luigi1 + # may or may not be genuinely installed in this env — popping + # ``sys.modules['luigi1']`` alone is not enough when luigi1 is on + # disk, since Python would re-import it from there. + import importlib + import sys + import types + + class _BlockLuigi1Finder: + """A meta_path finder that forces ``import luigi1`` to fail.""" + def find_spec(self, fullname, path, target=None): + if fullname == 'luigi1': + raise ImportError("luigi1 blocked by test") + return None + + saved_luigi1 = sys.modules.get('luigi1', None) + had_luigi1 = 'luigi1' in sys.modules + saved_meta_path = list(sys.meta_path) + blocker = _BlockLuigi1Finder() + try: + # Flag → False: stub luigi1 in sys.modules so import succeeds. + sys.modules['luigi1'] = types.ModuleType('luigi1') + sys.modules.pop('luigi.contrib._luigi1_compat', None) + sys.modules.pop('luigi.contrib.s3', None) + s3mod = importlib.import_module('luigi.contrib.s3') + self.assertIs(s3mod.S3Client, s3mod.S3ClientBoto1) + self.assertIs(s3mod.ReadableS3File, s3mod.ReadableS3FileBoto1) + + # Flag → True: install a meta_path finder that raises ImportError + # for ``luigi1``, so ``import luigi1`` fails even if the package + # is installed on disk. + sys.modules.pop('luigi1', None) + sys.meta_path.insert(0, blocker) + sys.modules.pop('luigi.contrib._luigi1_compat', None) + sys.modules.pop('luigi.contrib.s3', None) + s3mod = importlib.import_module('luigi.contrib.s3') + self.assertIs(s3mod.S3Client, s3mod.S3ClientBoto3) + self.assertIs(s3mod.ReadableS3File, s3mod.ReadableS3FileBoto3) + finally: + sys.meta_path[:] = saved_meta_path + if had_luigi1: + sys.modules['luigi1'] = saved_luigi1 + else: + sys.modules.pop('luigi1', None) + sys.modules.pop('luigi.contrib._luigi1_compat', None) + sys.modules.pop('luigi.contrib.s3', None) + importlib.import_module('luigi.contrib._luigi1_compat') + importlib.import_module('luigi.contrib.s3') From b4ffe574cd318739a6b383a86d0b77a01e074dae Mon Sep 17 00:00:00 2001 From: Felipe Velasquez Date: Wed, 6 May 2026 21:24:11 -0500 Subject: [PATCH 2/5] bump the version --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 9ec78a13d0..6d7182c924 100644 --- a/setup.py +++ b/setup.py @@ -57,7 +57,7 @@ def get_static_files(path): setup( name='luigi', - version='2.7.5+affirm.1.4.9.rc8', + version='2.7.5+affirm.1.4.9.rc10', description='Workflow mgmgt + task scheduling + dependency resolution', long_description=long_description, author='The Luigi Authors', From 796371ddc957c5757b3e553f0117edff2add3d43 Mon Sep 17 00:00:00 2001 From: Felipe Velasquez Date: Wed, 6 May 2026 23:01:03 -0500 Subject: [PATCH 3/5] increase coverage and fix some tests --- CLAUDE.md | 15 +++++++- PLAN_Py39_P312_DUAL_COMPATIBILITY.md | 2 ++ test/contrib/luigi1_compat_test.py | 13 +++++-- test/contrib/s3_test.py | 53 ++++++++++++++++++++++++---- 4 files changed, 72 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 20155f0a41..2ebc897510 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -151,7 +151,20 @@ cleanup is optional, not forced. uses the flag-resolved class, explicit `client=` overrides the default (same-stack always; cross-stack when both backing packages are installed), and the aliases flip in both directions when the flag is flipped via - `importlib.reload`. + `importlib.reload`. Two simple, hermetic tests pin the literal class + identity of `S3Target('s3://...').fs` for each flag value — each is gated + on whether the env naturally has the flag in that state, so coverage of + both directions comes from running under two different `uv` scenarios: + - `test_s3_target_default_fs_is_boto3_when_flag_true` — + `@unittest.skipUnless(IS_LUIGI1_DEPRECATED, ...)`. Constructs + `S3Target('s3://...')` with no `client=` and asserts `target.fs` is + an `S3ClientBoto3` (and **not** an `S3ClientBoto1`). Runs in any + scenario without `luigi1` installed (e.g. scenarios 2, 3, 4, 5). + - `test_s3_target_default_fs_is_boto1_when_flag_false` — + `@unittest.skipUnless(not IS_LUIGI1_DEPRECATED and HAS_BOTO_PKG, ...)`. + Same construction, asserts `target.fs` is an `S3ClientBoto1` (and + **not** an `S3ClientBoto3`). Runs in scenarios with `luigi1` and + `boto` installed (e.g. scenario 6). * The legacy `try: import boto / HAS_BOTO` setup at the top of `s3_test.py` is replaced by: - `USING_BOTO1 = S3Client is S3ClientBoto1` — does the flag point at the boto1 stack? diff --git a/PLAN_Py39_P312_DUAL_COMPATIBILITY.md b/PLAN_Py39_P312_DUAL_COMPATIBILITY.md index 3634b17e84..5d12ba0863 100644 --- a/PLAN_Py39_P312_DUAL_COMPATIBILITY.md +++ b/PLAN_Py39_P312_DUAL_COMPATIBILITY.md @@ -363,6 +363,8 @@ In this repo, no call site currently passes an explicit `client=S3ClientBoto1()` * Same-stack explicit `client=` override: an injected instance is preserved verbatim by `S3Target`. * Cross-stack explicit override: same assertion against the *other* stack — runs only when both backing packages are installed (skipped otherwise). * Aliases flip in both directions when `luigi.contrib._luigi1_compat` and `luigi.contrib.s3` are reloaded with `luigi1` stubbed in / out of `sys.modules`. Cleanup re-imports the real modules so subsequent tests aren't poisoned. + * `test_s3_target_default_fs_is_boto3_when_flag_true` — `@unittest.skipUnless(IS_LUIGI1_DEPRECATED, ...)`. Constructs `S3Target('s3://mybucket/key')` with no `client=` and asserts `target.fs` is an instance of `S3ClientBoto3` (and **not** `S3ClientBoto1`). Pins line 704 of `s3.py` (`self.fs = client or S3Client()`) to the boto3 stack when the flag is True. Runs in any `uv` scenario without `luigi1` installed. + * `test_s3_target_default_fs_is_boto1_when_flag_false` — `@unittest.skipUnless(not IS_LUIGI1_DEPRECATED and HAS_BOTO_PKG, ...)`. Same construction, asserts `target.fs` is an instance of `S3ClientBoto1` (and **not** `S3ClientBoto3`). Pins the same line to the boto1 stack when the flag is False. Runs in `uv` scenarios with `luigi1` and `boto` installed (scenario 6). Both flag states are covered by composing the env via `uv`, not by reloading modules in-process. * **`test/contrib/s3_test.py` setup change** — the legacy `try: import boto / HAS_BOTO` block is replaced by `USING_BOTO1 = S3Client is S3ClientBoto1`. Every existing `@unittest.skipIf(not HAS_BOTO, ...)` decorator is preserved verbatim as `@unittest.skipIf(not USING_BOTO1, ...)` — same skip semantics, now sourced from the flag rather than from probing `boto`. The previous `S3Client = S3ClientBoto3` rebinding (when `boto` was missing) is no longer needed because `S3Client` is already flag-resolved at import time. diff --git a/test/contrib/luigi1_compat_test.py b/test/contrib/luigi1_compat_test.py index 7e67c6c6be..d47034a305 100644 --- a/test/contrib/luigi1_compat_test.py +++ b/test/contrib/luigi1_compat_test.py @@ -74,10 +74,17 @@ def test_flag_is_bool(self): self.assertIsInstance(compat.IS_LUIGI1_DEPRECATED, bool) def test_flag_true_when_luigi1_unimportable(self): - # In the dev/CI env luigi1 is not installed. Make sure we evaluate the - # flag against that clean state. + # Force ``import luigi1`` to fail with ImportError regardless of + # whether luigi1 is genuinely installed on disk in this env. Just + # popping ``sys.modules['luigi1']`` is not enough when luigi1 is + # available — Python would re-import it from disk during reload. sys.modules.pop('luigi1', None) - compat = _force_reimport_compat() + finder = _RaiseOnImportFinder('luigi1', ImportError('luigi1 blocked by test')) + sys.meta_path.insert(0, finder) + try: + compat = _force_reimport_compat() + finally: + sys.meta_path.remove(finder) self.assertTrue( compat.IS_LUIGI1_DEPRECATED, "expected IS_LUIGI1_DEPRECATED == True when luigi1 cannot be imported", diff --git a/test/contrib/s3_test.py b/test/contrib/s3_test.py index 78acd3d8a5..d01b1413b4 100644 --- a/test/contrib/s3_test.py +++ b/test/contrib/s3_test.py @@ -83,11 +83,29 @@ # must skip in that degenerate case. S3CLIENT_INSTANTIABLE = HAS_BOTO_PKG if USING_BOTO1 else True -try: - from moto import mock_s3, mock_sts -except ImportError: - # moto >= 4.0 renamed mock_s3/mock_sts to mock_aws - from moto import mock_aws as mock_s3, mock_aws as mock_sts +if USING_BOTO1: + # boto1 makes HTTP calls via raw `http.client.HTTPSConnection`, which + # is not intercepted by the `responses` library that powers moto 1.x's + # default `mock_s3`. moto 1.x kept a separate HTTPretty-backed pair, + # `mock_s3_deprecated` / `mock_sts_deprecated`, that patches at the + # socket level and therefore catches boto1 traffic. moto 4+ dropped + # this entirely and no longer mocks boto1 at all (see the comment in + # run_tests.sh). + try: + from moto import mock_s3_deprecated as mock_s3, mock_sts_deprecated as mock_sts + except ImportError: + # No deprecated path on this moto. Best effort — the boto1 tests + # will leak to real AWS in this combo. + try: + from moto import mock_s3, mock_sts + except ImportError: + from moto import mock_aws as mock_s3, mock_aws as mock_sts +else: + try: + from moto import mock_s3, mock_sts + except ImportError: + # moto >= 5.0 renamed mock_s3 / mock_sts to mock_aws + from moto import mock_aws as mock_s3, mock_aws as mock_sts if (3, 4, 0) <= sys.version_info[:3] < (3, 4, 3): # spulec/moto#308 @@ -239,8 +257,15 @@ def test_init_with_config(self): @with_config({'s3': {'aws_role_arn': 'role', 'aws_role_session_name': 'name'}}) def test_init_with_config_and_roles(self): s3_client = S3Client() - self.assertEqual(s3_client.s3.access_key, 'AKIAIOSFODNN7EXAMPLE') - self.assertEqual(s3_client.s3.secret_key, 'aJalrXUtnFEMI/K7MDENG/bPxRfiCYzEXAMPLEKEY') + # The intent is that credentials came from `assume_role`, not from + # the kwargs. moto's STS mock returns temporary credentials whose + # access keys begin with the AWS-canonical "ASIA" prefix; secret + # keys are opaque, so we just check non-empty. + self.assertTrue( + s3_client.s3.access_key.startswith('ASIA'), + 'expected STS temp-credential prefix; got %r' % s3_client.s3.access_key, + ) + self.assertTrue(s3_client.s3.secret_key) def test_put(self): s3_client = S3Client(AWS_ACCESS_KEY, AWS_SECRET_KEY) @@ -649,6 +674,20 @@ def test_explicit_client_cross_stack_override(self): self.assertIs(target.fs, cross_stack) self.assertIsInstance(target.fs, other_cls) + @unittest.skipUnless(IS_LUIGI1_DEPRECATED, + 'flag is False in this env; pin the flag-True case via a uv scenario without luigi1') + def test_s3_target_default_fs_is_boto3_when_flag_true(self): + target = S3Target('s3://mybucket/key') + self.assertIsInstance(target.fs, S3ClientBoto3) + self.assertNotIsInstance(target.fs, S3ClientBoto1) + + @unittest.skipUnless(not IS_LUIGI1_DEPRECATED and HAS_BOTO_PKG, + 'flag is True or boto package is missing; pin the flag-False case via a uv scenario with luigi1 + boto') + def test_s3_target_default_fs_is_boto1_when_flag_false(self): + target = S3Target('s3://mybucket/key') + self.assertIsInstance(target.fs, S3ClientBoto1) + self.assertNotIsInstance(target.fs, S3ClientBoto3) + def test_aliases_flip_when_flag_flipped(self): # Reload luigi.contrib.s3 with luigi1 forcibly importable (flag → False) # and confirm the aliases flip to the boto1 stack; then with luigi1 From d0198635ab3c95172d177f745e2ca3bccf6f0483 Mon Sep 17 00:00:00 2001 From: Felipe Velasquez Date: Thu, 14 May 2026 18:10:44 -0500 Subject: [PATCH 4/5] lower urllib3 req to 1.25.9, bump luigi version --- CLAUDE.md | 38 ++++++++++++++++++++++++++++ PLAN_Py39_P312_DUAL_COMPATIBILITY.md | 4 +-- setup.py | 4 +-- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2ebc897510..988523c45c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,6 +98,44 @@ python -m pytest test/ --ignore=test/contrib/mysqldb_test.py --ignore=test/visua - External `six` package (e.g. `from six.moves.urllib...`) → native `urllib.*` (Python 3.0+) - `six.PY3` checks can be removed entirely — always `True` on any supported Python 3.x +### Py312 Dependency Constraints in `setup.py` + +The Py312 markers in `install_requires` (`; python_version >= "3.12"`) exist so the published wheel resolves cleanly on Py312 *without* forcing Py39 DTs to bump anything. Keep the lower bounds as lenient as possible — this package does not own the dependency pins of downstream DTs. + +- `tornado>=6.0` — Py312 dropped support for tornado 5.x's `asyncio` shim. +- `requests>=2.31` — first release that ships Py312 wheels. +- `urllib3>=1.25.9` — **lenient on purpose at the resolver level**. `1.25.9` is the minimum that `requests>=2.31` resolves against. Do **not** bump this to `>=2.0` — forcing DTs to take urllib3 2.x across the fleet is out of scope for luigi; each DT decides its own urllib3 pin (e.g. via its own `requirements.txt`). + - **Caveat — the importable minimum on Py312 is actually `1.26.5`, not `1.25.9`.** All urllib3 1.25.x and 1.26.0–1.26.4 releases bundle `urllib3.packages.six.moves`, which crashes at `import urllib3` time on Py312 with `ModuleNotFoundError: No module named 'urllib3.packages.six.moves'` (Py312's hardened import system rejects the lazy-stub trick the bundled `six` uses). Bisected on 2026-05-14 against `--python 3.12` via `uv run`: `1.25.10`/`1.26.0`/`1.26.1`/`1.26.3`/`1.26.4` all fail to import; `1.26.5` is the first version that imports cleanly (urllib3 1.26.5 release notes: "Removed dependencies on `six`"). The marker is left at `>=1.25.9` deliberately — bumping the floor to `1.26.5` would force every DT to re-resolve urllib3, which is exactly what the lenient-marker policy is trying to avoid. DTs that actually run on Py312 are expected to pin `urllib3>=1.26.5` themselves (most already do, transitively, via `requests>=2.31` → modern `botocore` → `urllib3>=1.26.5`). If a DT ever resolves a Py312 install to urllib3 in `[1.25.9, 1.26.5)`, that's a DT-side resolver bug, not a luigi bug. +- `setuptools>=68`, `packaging>=23` — needed by Py312 build / metadata tooling. + +When in doubt, set the Py312 lower bound to the *minimum* version that imports and runs on Py312, not the latest available. + +### Latent bug: `luigi/rpc.py` `HAS_REQUESTS=False` branch is unreachable + +`luigi/rpc.py:39-46` wraps the `requests` import in a `try/except ImportError` and sets `HAS_REQUESTS=False` on failure: + +```python +try: + import requests_unixsocket as requests +except Exception: + HAS_UNIX_SOCKET = False + try: + import requests + except ImportError: + HAS_REQUESTS = False +``` + +…but then unconditionally references `requests.adapters.HTTPAdapter` at module scope a few lines later: + +```python +class BobaPKIHTTPAdapter(requests.adapters.HTTPAdapter): + ... +``` + +If `requests` actually fails to import (e.g. because the *installed* `urllib3` doesn't import on Py312 — see urllib3 caveat above), `HAS_REQUESTS` is set to `False` but the class definition still runs and crashes the entire `import luigi` with `NameError: name 'requests' is not defined`. The `HAS_REQUESTS` flag is therefore dead state — `luigi` cannot actually be imported without `requests`. + +This is **not** worth fixing reactively (luigi has hard-required `requests` for years; nobody installs it without `requests`). The bug only surfaces in pathological scenarios like "Py312 + `urllib3==1.25.10` (resolves but doesn't import) → `requests` import fails silently → luigi import dies on the unrelated `BobaPKIHTTPAdapter` line, masking the real urllib3 problem". Documented here so the next person who hits a confusing `NameError: name 'requests' is not defined` on Py312 doesn't waste time chasing a phantom — the real fault is almost certainly upstream of `requests` (urllib3, charset_normalizer, idna, certifi, etc.). Fix: either drop the dead `try/except` (luigi-side), or pin a known-importable `urllib3` (env-side). + ### `IS_LUIGI1_DEPRECATED` flag (boto1 ↔ boto3 dispatch) `luigi/contrib/_luigi1_compat.py` exposes a single private flag: diff --git a/PLAN_Py39_P312_DUAL_COMPATIBILITY.md b/PLAN_Py39_P312_DUAL_COMPATIBILITY.md index 5d12ba0863..7877e1aebe 100644 --- a/PLAN_Py39_P312_DUAL_COMPATIBILITY.md +++ b/PLAN_Py39_P312_DUAL_COMPATIBILITY.md @@ -21,7 +21,7 @@ All changes in PR #28 use standard Python 3 APIs available since Python 3.3. Non | `importlib.resources.files` | `server.py` | Python 3.9 | | `LuigiRunResult.worker` attribute access | `retcodes.py` | N/A — logic change | | `tornado>=6.0,<7` | `setup.py` | Supports Python 3.6+ | -| `urllib3>=2.0` | `setup.py` | Supports Python 3.8+ | +| `urllib3>=1.25.9` | `setup.py` | Supports Python 3.5+; lenient lower bound — DTs choose their own pin | --- @@ -149,7 +149,7 @@ This branch was cut from `2.7.5.affirm.1.4.9` instead of `2.7.5+affirm.1.4.7`. T #### Dependency version constraints * `tornado>=6.0,<7` — verified: `6.5.5` installed and working on Py312 -* `urllib3>=2.0` — verified: `2.6.3` installed and working on Py312 +* `urllib3>=1.25.9` — lenient lower bound. Py312 itself only requires `urllib3>=1.21.1` to run; we set `>=1.25.9` to match the minimum that `requests>=2.31` resolves cleanly against. Pinning `>=2.0` was rejected because forcing DTs to bump urllib3 across the fleet is out of scope for this package — each DT decides its own urllib3 pin. Verified `2.6.3` installs and works on Py312; older 1.26.x also resolves. * `setuptools>=68` and `packaging>=23` — verified: `setuptools 82.0.1`, `packaging 26.0` on Py312 * Confirm all of the above install correctly on Py39 environments used by DTs — verified via `.venv39` (pyenv 3.9.18): `tornado 6.5.5`, `urllib3 2.6.3`, `setuptools 82.0.1`, `packaging 26.0` all install and import cleanly diff --git a/setup.py b/setup.py index 6d7182c924..f9e6e9ebbd 100644 --- a/setup.py +++ b/setup.py @@ -42,7 +42,7 @@ def get_static_files(path): 'tornado>=6.0 ; python_version >= "3.12"', 'python-daemon<3.0', 'requests>=2.31 ; python_version >= "3.12"', - 'urllib3>=2.0 ; python_version >= "3.12"', + 'urllib3>=1.25.9 ; python_version >= "3.12"', 'setuptools>=68 ; python_version >= "3.12"', 'packaging>=23 ; python_version >= "3.12"', 'enum34>1.1.0 ; python_version < "3.4"' @@ -57,7 +57,7 @@ def get_static_files(path): setup( name='luigi', - version='2.7.5+affirm.1.4.9.rc10', + version='2.7.5+affirm.1.4.9.rc11', description='Workflow mgmgt + task scheduling + dependency resolution', long_description=long_description, author='The Luigi Authors', From cb16abd0cd269085c162c2f24de5d3e320ee82df Mon Sep 17 00:00:00 2001 From: Felipe Velasquez Date: Tue, 16 Jun 2026 16:20:31 -0500 Subject: [PATCH 5/5] drop .rc11, bump to version 2.7.5+affirm.1.4.10 --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index f9e6e9ebbd..6bafbac8eb 100644 --- a/setup.py +++ b/setup.py @@ -57,7 +57,7 @@ def get_static_files(path): setup( name='luigi', - version='2.7.5+affirm.1.4.9.rc11', + version='2.7.5+affirm.1.4.10', description='Workflow mgmgt + task scheduling + dependency resolution', long_description=long_description, author='The Luigi Authors',