Fix Runner packaging for ad hoc scripts#471
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Runner packaging mechanism to generate pyproject.toml in-memory and write it directly into the tar archive, preventing mutation of the caller's current working directory. It also adds corresponding unit and integration tests. The review feedback highlights two important issues: first, manually created tarfile.TarInfo objects default to permissions of 000, which will cause permission denied errors upon extraction unless tarinfo.mode is explicitly set (e.g., to 0o644). Second, generating a pyproject.toml when legacy metadata (setup.py or setup.cfg) is present can conflict with the existing configuration, so it is recommended to skip generation and log a warning instead.
| if generated_pyproject is not None: | ||
| data = generated_pyproject.encode("utf-8") | ||
| tarinfo = tarfile.TarInfo("pyproject.toml") | ||
| tarinfo.size = len(data) | ||
| tar.addfile(tarinfo, io.BytesIO(data)) |
There was a problem hiding this comment.
When creating a tarfile.TarInfo object manually using tarfile.TarInfo("pyproject.toml"), its fields are initialized to 0 or empty strings. This means tarinfo.mode defaults to 0 (i.e., permissions --------- or 000).
When this archive is extracted on the executor side, the extracted pyproject.toml file will have no read/write permissions, which will cause permission denied errors when pip or uv attempts to read it during installation.
To fix this, explicitly set tarinfo.mode = 0o644 (standard read/write permissions for regular files) before adding it to the archive.
| if generated_pyproject is not None: | |
| data = generated_pyproject.encode("utf-8") | |
| tarinfo = tarfile.TarInfo("pyproject.toml") | |
| tarinfo.size = len(data) | |
| tar.addfile(tarinfo, io.BytesIO(data)) | |
| if generated_pyproject is not None: | |
| data = generated_pyproject.encode("utf-8") | |
| tarinfo = tarfile.TarInfo("pyproject.toml") | |
| tarinfo.size = len(data) | |
| tarinfo.mode = 0o644 | |
| tar.addfile(tarinfo, io.BytesIO(data)) |
| has_legacy_metadata = os.path.exists(os.path.join(cwd, "setup.py")) or os.path.exists(os.path.join(cwd, "setup.cfg")) | ||
| if has_legacy_metadata and not self._dependencies: | ||
| logger.debug("Python package metadata already exists, skipping generated metadata") | ||
| return None |
There was a problem hiding this comment.
If the project already contains legacy metadata files like setup.py or setup.cfg, generating a pyproject.toml with py-modules = [] when self._dependencies is specified will override or conflict with the existing setup. This can prevent setuptools from discovering and packaging the actual modules/packages defined in the legacy metadata.
If legacy metadata exists, we should skip generating pyproject.toml entirely and log a warning advising the user to specify their dependencies in their existing setup.py or setup.cfg.
| has_legacy_metadata = os.path.exists(os.path.join(cwd, "setup.py")) or os.path.exists(os.path.join(cwd, "setup.cfg")) | |
| if has_legacy_metadata and not self._dependencies: | |
| logger.debug("Python package metadata already exists, skipping generated metadata") | |
| return None | |
| has_legacy_metadata = os.path.exists(os.path.join(cwd, "setup.py")) or os.path.exists(os.path.join(cwd, "setup.cfg")) | |
| if has_legacy_metadata: | |
| if self._dependencies: | |
| logger.warning( | |
| "Python package metadata (setup.py/setup.cfg) already exists. " | |
| "Skipping pyproject.toml generation to avoid conflicting with existing metadata. " | |
| "Please specify dependencies in your setup.py or setup.cfg." | |
| ) | |
| else: | |
| logger.debug("Python package metadata already exists, skipping generated metadata") | |
| return None |
051144a to
7fdbb6f
Compare
Summary
uv pip install .flmexecrunningRunner(...)without dependenciesVerification
uv pip install --target deps .Flame Runner works perfectly! Results: [100, 400]python3 -m pytest -vv -s --durations=0 tests/test_flmexec.py