diff --git a/CLAUDE.md b/CLAUDE.md index e0041b1a2f..988523c45c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,6 +98,133 @@ 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: + +- `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`. 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? + - `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..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 @@ -265,6 +265,147 @@ 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_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. + +### 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/setup.py b/setup.py index 9ec78a13d0..6bafbac8eb 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.rc8', + version='2.7.5+affirm.1.4.10', description='Workflow mgmgt + task scheduling + dependency resolution', long_description=long_description, author='The Luigi Authors', diff --git a/test/contrib/luigi1_compat_test.py b/test/contrib/luigi1_compat_test.py new file mode 100644 index 0000000000..d47034a305 --- /dev/null +++ b/test/contrib/luigi1_compat_test.py @@ -0,0 +1,123 @@ +# -*- 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): + # 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) + 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", + ) + + 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..d01b1413b4 100644 --- a/test/contrib/s3_test.py +++ b/test/contrib/s3_test.py @@ -24,26 +24,88 @@ 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 -try: - import boto - from boto.exception import S3ResponseError - from boto.s3 import key - HAS_BOTO = 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 +# `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: - from moto import mock_s3, mock_sts + from boto.exception import S3ResponseError as _Boto1ResponseError + from boto.s3 import key + HAS_BOTO_PKG = True 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 + _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 + +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 @@ -54,6 +116,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 +135,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 +160,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 +198,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 +225,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,19 +246,26 @@ 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() - 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) @@ -200,7 +273,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 +286,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 +302,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 +316,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 +330,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 +344,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 +604,139 @@ 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) + + @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 + # 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')