Repository navigation
ci: build and test conda packages and wheels - #378
jharlow-intel wants to merge 16 commits into
Conversation
Adds what Intel Python CI (gh-ci) needs: ci/, a noarch conda-forge recipe and .github/workflows/intel-ci.yml. The workflow builds the conda package and the pure Python wheel for each Python in the recipe and pyproject.toml, on Linux, and runs dokimi/tests against each installed package with ci/test.py. Intel's Jenkins tests and scans the same packages again. release.yml stays the release process. The recipe's version, Python floor, console script and run requirements are checked against pyproject.toml by tests/test_conda_recipe.py. The wheel matrix follows the recipe's platforms, Linux. Signed-off-by: Harlow, Jordan <jordan.harlow@intel.com>
|
OpenCodeReview: Commit 09ac3d0 was not reviewed. This PR has reached its review limit (6 reviews). CI at `09ac3d0`: 11 still running, 8 passing, 2 skipped
1 more not listed. To review this commit: re-request review from exai-bot in the Reviewers list. <details><summary>Last review (ccb25d4): no findings</summary> Open threads from the bot: 1 </details> |
| group: intel-ci-${{ github.ref }} | ||
| cancel-in-progress: ${{ github.event_name == 'pull_request' }} |
There was a problem hiding this comment.
Must-fix — Impact: a push to main can lose its entire build/test run, leaving no artifacts for Jenkins for that commit. Trigger: three main pushes while the first run is still active. Evidence: all main pushes share this concurrency group; GitHub concurrency keeps only one pending run and replaces that pending run when another arrives, even with cancel-in-progress false. The existing ci.yml explicitly avoids this behavior by using github.run_id for non-PR events. Workaround: group PR runs by PR number/ref, but use github.run_id for push/tag runs. Severity justification: medium because this is a bounded but material omission of the intended per-commit CI execution.
There was a problem hiding this comment.
Agreed. Fixed in c60220d: the group is now ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}, the same as ci.yml, so every push and tag run has a group of its own. tests/test_intel_ci.py asserts intel-ci.yml's concurrency equals ci.yml's.
| built = sorted( | ||
| path | ||
| for path in output.iterdir() | ||
| if path.name.endswith(PACKAGE_SUFFIXES) | ||
| ) |
There was a problem hiding this comment.
Must-fix — Impact: stale packages are reported as current build outputs and can cause conflicting installations; wheel metadata rewriting also applies to old wheels. Trigger: run the documented build command into the same --output directory before and after changing the package version, then pass that directory to hooks.py. Evidence: collect() enumerates every package in output, not just those moved from scratch; hooks.install() installs all matching artifacts, so two versions of dokimi are requested together. Workaround: use a fresh output directory for every build. Return only the paths moved by this invocation, or reject a nonempty output directory, and add a reused-output regression test. Severity justification: medium because this is a bounded artifact-integrity and local CI failure, not a failure of the workflow's fresh-output path.
There was a problem hiding this comment.
Reproduced with the real CLI: a second ci/build.py --system wheel into the same --output printed build: dokimi-0.0.1-py3-none-any.whl, dokimi-0.1.0-py3-none-any.whl. Fixed in 544dd10: collect() returns only the packages this run moved, so the report and the wheel-requires rewrite cover only this build (dokimi declares no wheel-requires, so the rewrite half was latent here). The upload glob and hooks.py still take every package in the directory; the docstring now says to give each build an empty --output. Refusing a non-empty directory would also cover those, but it changes how Jenkins may call the shared script, so that stays your call.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #378 +/- ##
==========================================
+ Coverage 94.40% 94.78% +0.37%
==========================================
Files 313 353 +40
Lines 40216 46575 +6359
Branches 6822 7974 +1152
==========================================
+ Hits 37966 44146 +6180
- Misses 1435 1541 +106
- Partials 815 888 +73 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
*.condaartifacts, butconda-buildcan emit.tar.bz2packages… · Newskipifmark won’t prevent module-level code from executing during test collection. In… · New' = 'is brittle: conda entry points… · Newgitconfig… · New
What changed in this PR
Adds Intel Internal CI (“gh-ci”) infrastructure to build/test dokimi’s conda package and pure-Python wheels in GitHub Actions, producing artifacts that Intel Jenkins can retest/scan.
Changes:
- Add conda-build recipe + config to build a
noarch: pythonpackage (Linux-only) and keep it synchronized withpyproject.toml. - Introduce shared CI scripts (
ci/matrix.py,ci/build.py,ci/hooks.py,ci/test.py) to build artifacts and run the test suite against the installed package. - Add a dedicated GitHub Actions workflow (
.github/workflows/intel-ci.yml) and CI configuration (ci/config.yml) to run the matrix and tests.
| File | Description |
|---|---|
| dokimi/tests/test_conda_recipe.py | Adds tests to ensure conda recipe metadata stays aligned with pyproject.toml. |
| conda-recipe/meta.yaml | Introduces the noarch conda recipe used by gh-ci/Jenkins builds. |
| conda-recipe/conda_build_config.yaml | Declares the Python versions for conda package builds. |
| ci/test.py | Runs pytest from the checkout against the installed package and validates import/data-file behavior. |
| ci/matrix.py | Derives build matrix (platforms + Python versions) from recipe/pyproject declarations. |
| ci/hooks.py | Installs built artifacts into fresh envs and runs ci/test.py in an isolated way. |
| ci/config.yml | Defines artifact naming and test dependencies for gh-ci. |
| ci/build.py | Builds conda packages or wheels per version and collects artifacts for upload. |
| ci/README.md | Documents how gh-ci works for this repository. |
| .github/workflows/intel-ci.yml | Adds the Intel Python CI workflow to build/test conda packages and wheels. |
Review effort: Lite
| - uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 | ||
| with: | ||
| name: dokimi ${{ matrix.name }} Python ${{ matrix.python }} | ||
| path: ${{ runner.temp }}/output/*.conda |
There was a problem hiding this comment.
Not an issue now: the workflow installs conda-build from conda-forge (26.9.1 today), whose default package format is .conda (Config().conda_pkg_format is CondaPkgFormat.V2), and if-no-files-found: error would make a .tar.bz2 build fail loudly rather than drop it.
| pytestmark = pytest.mark.skipif(not _RECIPE.is_file(), reason="not a checkout: the recipe is next to the project") | ||
|
|
||
| _SCRIPT = _PROJECT / "scripts" / "conda_forge_specs.py" | ||
| _spec = importlib.util.spec_from_file_location("conda_forge_specs", _SCRIPT) | ||
| assert _spec and _spec.loader | ||
| cfs = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(cfs) |
There was a problem hiding this comment.
Does not reproduce: with only dokimi/ copied (no conda-recipe/ next to it) the module imports scripts/conda_forge_specs.py without error and both tests are skipped (2 skipped). A layout without scripts/ would also break the other test files that load a script at module level, so this follows the suite's convention.
| recipe, project = _recipe(), _project() | ||
| assert str(recipe["package"]["version"]) == project["version"] | ||
| assert f"python {project['requires-python']}" in recipe["requirements"]["run"] | ||
| entries = dict(e.split(" = ") for e in recipe["build"]["entry_points"]) |
There was a problem hiding this comment.
The recipe in this repo spells every entry point name = module:func, and a different spelling makes the test fail with a ValueError, not pass silently. Left as is.
| with tempfile.TemporaryDirectory() as home: | ||
| # Tests that commit to scratch repositories need a git identity. | ||
| gitconfig = pathlib.Path(home) / "gitconfig" | ||
| gitconfig.write_text("[user]\n\tname = gh-ci\n\temail = gh-ci@example.com\n") |
There was a problem hiding this comment.
The content is ASCII, so every default encoding writes the same bytes. Left as is.
napetrov
left a comment
napetrov
left a comment
There was a problem hiding this comment.
Only findings not already raised on this PR (stale packages in collect(), main-push concurrency, .tar.bz2 upload and the Copilot items are already covered and omitted). Found by automated haiku review passes; the three nit: threads were reproduced with a local probe on head, the question: threads were read, not run.
| return pathlib.Path(shown.stdout.strip()).resolve() | ||
|
|
||
|
|
||
| CHECK_DATA = ( |
There was a problem hiding this comment.
question: a wheel missing products.yaml or intents.yaml still passes gh-ci
CHECK_DATA checks only DEFAULT_PRODUCTS_PATH and DEFAULT_SCHEMA_PATH, but it replaces the deselected test_default_data_files_ship_in_the_wheel, which covers four files; registry._DEFAULT_REGISTRY and intents._DEFAULT_INTENTS are dropped.
Fix: add those two paths to paths. (Read, not run.)
There was a problem hiding this comment.
Confirmed: CHECK_DATA printed two of the four paths. Fixed in a5c235e: it now checks registry._DEFAULT_REGISTRY and intents._DEFAULT_INTENTS as well, and tests/test_intel_ci.py compares its output with the four files of the deselected test. Moving intents.yaml or products.yaml aside now makes the check exit 1.
| "s390x": False, | ||
| }, | ||
| } | ||
| CONDA_SKIP_RE = re.compile(r"\s*skip\s*:\s*(?:true|True|yes)\s*#\s*\[(.+)\]\s*") |
There was a problem hiding this comment.
nit: a skip line with a trailing comment keeps the skipped platform in the matrix
CONDA_SKIP_RE requires the line to end at ].
How to check: from_conda_recipe on skip: true # [win] # windows not supported → ['linux','windows','macos'] (# [win] alone → ['linux','macos']).
Expected: windows excluded, as conda-build does. Fix: drop the end anchor / allow \s*(#.*)?$.
There was a problem hiding this comment.
Reproduces (skip: true # [win] # windows not supported keeps windows; conda-build applies that selector). Latent for dokimi, whose only skip is # [not linux] with no trailing comment. matrix.py is a shared gh-ci script, so the fix belongs in the shared copy; left to the author.
| ) | ||
| CLASSIFIER_RE = re.compile(r"Programming Language :: Python :: 3\.(\d+)") | ||
| FREE_THREADING_CLASSIFIER = "Programming Language :: Python :: Free Threading" | ||
| LOWER_BOUND_RE = re.compile(r">=\s*3\.(\d+)") |
There was a problem hiding this comment.
nit: requires-python = ">3.10" still yields a 3.10 cell; upper bounds ignored
LOWER_BOUND_RE only reads >=3.N.
How to check: from_pyproject with >3.10 and a 3.10 classifier → ['3.10'].
Expected: []. Fix: filter classifiers with packaging.specifiers.SpecifierSet.
There was a problem hiding this comment.
Reproduces (>3.10 with a 3.10 classifier gives ['3.10']). For the >= form the script raises a DeclarationError rather than filtering, so the matching result here would be that error, not []. Latent: dokimi declares >=3.11. Shared script, left to the author.
| CONDA_SKIP_RE = re.compile(r"\s*skip\s*:\s*(?:true|True|yes)\s*#\s*\[(.+)\]\s*") | ||
| OS_CLASSIFIERS = { | ||
| "Operating System :: OS Independent": tuple(PLATFORMS), | ||
| "Operating System :: POSIX": ("linux",), |
There was a problem hiding this comment.
nit: a FreeBSD-only classifier produces Linux cells
OS_CLASSIFIERS prefix-matches Operating System :: POSIX.
How to check: from_os_classifiers with Operating System :: POSIX :: BSD :: FreeBSD → ['linux'].
Expected: no Linux cell. Fix: exact match for the bare POSIX key.
There was a problem hiding this comment.
Reproduces (Operating System :: POSIX :: BSD :: FreeBSD gives ['linux']). Latent: dokimi's pyproject.toml names no Operating System. Shared script, left to the author.
| build: | ||
| name: ${{ matrix.name }} Python ${{ matrix.python }} | ||
| needs: matrix | ||
| runs-on: ${{ vars.GH_CI_RUNNER || (github.repository_owner == 'intel' && 'intel-ubuntu-latest' || 'ubuntu-latest') }} |
There was a problem hiding this comment.
question: Windows/macOS matrix cells would build on the Linux runner
build_matrix can emit non-Linux cells (recipe without not linux skip, or OS classifiers), but every job's runs-on ignores matrix.platform, so such a cell builds Linux packages under a Windows/macOS artifact name. Latent today. Should runs-on map from matrix.platform, or matrix.py reject non-Linux?
There was a problem hiding this comment.
Latent: the recipe skips not linux and pyproject.toml names no Operating System, so matrix.py emits Linux cells only. Mapping runs-on from matrix.platform or refusing non-Linux cells is a design call for the author.
| record = metadata.removesuffix("METADATA") + "RECORD" | ||
| for index, (info, data) in enumerate(members): | ||
| if info.filename == metadata: | ||
| head, blank, body = data.decode("utf-8").partition("\n\n") |
There was a problem hiding this comment.
question: CRLF METADATA would put Requires-Dist after the body
partition("\n\n") finds no separator with CRLF, so the whole file is treated as the header and the added requirements land after the description, where installers ignore them. Latent (pip writes LF); splitting on \r?\n\r?\n would cover it.
There was a problem hiding this comment.
Correct for CRLF metadata (partition("\n\n") finds no separator). Latent twice over: pip and setuptools write LF, and dokimi declares no [tool.gh-ci] wheel-requires, so add_requirements never runs for dokimi. Left to the author for the shared script.
The recipe added in this PR repeats the version, and test_conda_recipe.py fails a release PR that bumps only pyproject.toml, as step 1 said to. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The wheel cells resolved every dependency from PyPI, so litellm 1.108.0 failed 16 tests on any pull request that changes no dependency. Every event but a tag now installs dokimi/requirements.lock.txt with --no-deps --require-hashes, then the wheel with --no-deps. A tag still tests what a user of the release gets. The conda cells are unchanged: conda-forge does not carry the lock's versions, so the lock cannot pin them. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
CodeFactor's bandit flags B310 on the urlopen, which fetches a fixed https URL and checks the binary against a pinned sha256. The nosec matches the other urlopen calls in the repo. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
36bcbae: CodeFactor's one finding was bandit B310 on the rattler-build |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 36bcbae. The recipe, the version sync and the lock-based wheel install read well. Three things block merge: the repo's own make ci-local crashes on the new workflow, main pushes can lose their run, and the data-file check in ci/test.py is weaker than the test it replaces.
What I ran, on a Linux review host at 36bcbae:
make ci-local: aborts before running any step (see the inline comment on the matrix).tests/test_conda_recipe.py: 2 passed. Mutating the recipe's version, Python floor, entry point and run requirements: 22 variants, all caught.zizmoronintel-ci.yml: no findings.pip wheelplusadd_requirementson the real project gives a valid wheel with the twoRequires-Distlines.
Open bot points I confirm:
- I agree with exai-bot that the
concurrencygroup on main pushes can drop a run. New evidence:ci.yml:26-29says the same and usesgithub.run_idfor non-PR events;intel-ci.yml:16-18keys ongithub.ref, so every main push shares one group.
Waiting on the author or a decision: @napetrov's question on ci/test.py:49 (the wheel check covers two of the four data files) is still unfixed at this head. The comment above CHECK_DATA says the deselected test is the only check of these files, but test_default_data_files_ship_in_the_wheel has four parameters (products.yaml, intents.yaml, DEFAULT_PRODUCTS_PATH, DEFAULT_SCHEMA_PATH) and CHECK_DATA checks two. A wheel without products.yaml or intents.yaml passes gh-ci. @napetrov's questions on runs-on ignoring matrix.platform and on CRLF METADATA wait for the author.
Checked, no issues
- Commits: all four carry a
Signed-off-by. - Repo rules: no changelog fragment is needed (nothing under
dokimi/dokimi/,dokimi/data/orpyproject.tomlchanges);releasing.mdis already in the nav; SPDX headers are present; no generated artifacts. - Lock install: every requirement in
ci/config.ymlis inrequirements.lock.txtat a satisfying version.
Bot points I checked that no longer apply at this head (4)
- exai-bot's point on stale packages in
collect()does not apply to the workflow: every build job writes to a fresh$RUNNER_TEMP/output, so only a local rerun into a reused--outputis affected. - Copilot's point on the
*.condaupload glob does not apply: the job installs the latest conda-build from conda-forge, which writes.conda, and the fork run in the description uploaded all four artifacts underif-no-files-found: error. - Copilot's point on the module-level import in
test_conda_recipe.pydoes not apply:scripts/conda_forge_specs.pysits next totests/in every layout where the tests run, andci/test.pyruns them from the checkout. - Copilot's point on splitting entry points on
' = 'does not apply: the test compares againstmeta.yaml, so a different spacing fails loudly instead of passing silently.
intel-ci.yml's fromJSON matrix is a string: ci_pythons() crashed on it, so make ci-local could not run. Such a job builds packages, so it is reported as a skip. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
One group per ref let a third main push replace the pending second run, so Jenkins never got that commit's packages. Group as ci.yml does: by PR number, else run_id. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
collect() listed every package in --output, so a reused one reported an earlier build's packages as this build's and added wheel-requires to them again. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…st covers products.yaml and intents.yaml were dropped, so a wheel missing either still passed. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Second round on the review threads, on top of 36bcbae. Four commits pushed to the branch, one per defect, each with a test that fails at 36bcbae:
On the head and merged with main (Python 3.13, Linux): the full suite, ruff, mypy, bandit, zizmor, the benchmark gate, check_comment_blocks, check_growth and the changelog check all pass, and Still open, for the author:
The copilot threads (module-level import, |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a5c235e. The four fixes since my last review hold: the fromJSON matrix no longer crashes make ci-local, main pushes keep their own concurrency group, collect() lists only what it built, and the data check covers all four files of the test it replaces. One nit inline.
What I ran, on a Linux review host at a5c235e:
make ci-local: 31 passed, 0 failed, 11 skipped. The fourintel-ci.ymljobs with afromJSONmatrix are reported as skips; thematrixjob runs and passes. Changed-line coverage passes.- Mutation: reverting the
collect()change, dropping the two added paths from the data check, and reverting the concurrency group each make a test intest_intel_ci.pyfail. Reverting either half of theci_local.pyfix makes the newtest_ci_local.pycase fail.
Waiting on the author or a decision: @napetrov's questions on runs-on ignoring matrix.platform and on CRLF METADATA, and the three matrix.py points (skip line with a trailing comment, >3.10, FreeBSD classifier), are latent for dokimi and live in the shared gh-ci scripts. They wait for the author.
Checked, no issues
- Commits: all eight carry a
Signed-off-by. - Data check:
CHECK_DATAlists the same four paths as the parameters of the deselected test. - Concurrency: the group expression is identical to
ci.yml:28.
Bot points I checked that no longer apply at this head (5)
- exai-bot's point on the shared
concurrencygroup does not apply:intel-ci.yml:18now uses ci.yml'spull_request.number || run_id, and a test pins the two equal. - exai-bot's point on stale packages in
collect()does not apply to what it reports:collect()now returns only the packages this run moved, and every workflow build job writes to a fresh output directory. - Copilot's point on the
*.condaupload glob does not apply: conda-build from conda-forge writes.conda, andif-no-files-found: errorfails loudly on any other format. - Copilot's point on the module-level import in
test_conda_recipe.pydoes not apply:scripts/conda_forge_specs.pysits next totests/wherever the tests run. - Copilot's point on splitting entry points on
' = 'does not apply: the test compares withmeta.yaml, so another spelling fails loudly instead of passing.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 21b51d5, which fix the nit from my review:
21b51d5:dokimi/scripts/ci_local.py:30: the docstring now saysbuild and test jobs, matching the fourfromJSONjobs that are skipped.
make ci-local at 21b51d5: 31 passed, 0 failed, 11 skipped.
Not approving yet. Still open: @napetrov on runs-on ignores matrix.platform: map it or make matrix.py reject non-Linux cells (design call, latent); @napetrov on CRLF METADATA in build.py add_requirements and the three matrix.py points (trailing-comment skip, >3.10, FreeBSD classifier) are in the shared gh-ci scripts, latent for dokimi.
… and POSIX as declared intel-ci.yml runs every job on a Linux runner, so BUILT_PLATFORMS is Linux and a declared Windows or macOS platform is reported and left out instead of getting a cell. A skip selector may carry a trailing comment, as in conda-build's sel_pat. Every requires-python clause now counts: a classifier it excludes on its newest patch release is an error, as >= already was (>3.10 still admits 3.10.1). A POSIX sub-classifier other than POSIX :: Linux, such as FreeBSD, no longer means Linux. Signed-off-by: Harlow, Jordan <jordan.harlow@intel.com>
partition("\n\n") found no blank line in CRLF METADATA, so the Requires-Dist lines
landed after the description, where installers ignore them.
Signed-off-by: Harlow, Jordan <jordan.harlow@intel.com>
|
My open points here do not block this PR. They are latent for dokimi and live in the shared gh-ci scripts, so they move to #385:
From my side, nothing is left open on #378. |
Signed-off-by: Harlow, Jordan <jordan.harlow@intel.com>
f2e1123 to
ccb25d4
Compare
Signed-off-by: Harlow, Jordan <jordan.harlow@intel.com>
| - "markdown-it-py >=3" | ||
| - packaging |
There was a problem hiding this comment.
CI check Test Linux Python 3.11 fails: The new conda environment installs ci/config.yml test.requires, which omits mypy (a project dev dependency). This causes the typecheck test collection error and the repeated mypy version-probe failures. Proposed fix, not run by the bot.
Suggestion:
| - "markdown-it-py >=3" | |
| - packaging | |
| - "markdown-it-py >=3" | |
| - packaging | |
| - "mypy >=1.8" |
napetrov
left a comment
napetrov
left a comment
There was a problem hiding this comment.
Verdict: comment only. No blocking thread; four nits, inline. Head ab77902, merge base 3258d6c, lane L3. Earlier threads re-checked on head: most are fixed, four are still open (below).
CI: 2 failing jobs, Intel CI / Test Linux Python 3.11 and 3.14, with no base run of this workflow to compare. Their logs name tests/test_repo_l0_typecheck.py, which is in neither tree, so CI may have tested a different tree than head. Question: which commit did those cells check out? Please re-run them on ab77902 after that is settled; I could not attribute the failures to the PR.
| # | Finding | Label | Provenance | Scope | Probe |
|---|---|---|---|---|---|
| 1 | a build under a broken directory above scratch loses its package |
nit | introduced | local | probes/out-p3_collect.txt |
| 2 | a non-ASCII digit in a classifier is read as a Python version | nit | introduced | local | probes/out-p6_b2_b4.txt |
| 3 | config.yml workflow and artifacts keys are read by nothing (C3) |
nit | introduced | local | probes/out-b3.txt |
| 4 | a non-UTF-8 pyproject.toml ends the matrix job with a traceback |
nit | introduced | local | probes/out-b4.txt |
Raised before (status on head):
- @copilot-pull-request-reviewer, test_conda_recipe.py line 24: still open. Our probe reproduces it: collection aborts with FileNotFoundError when scripts/ is absent (probes/out-t4.txt).
- @copilot-pull-request-reviewer, upload glob, intel-ci.yml line 78: still open. Its entry-point split and gitconfig encoding threads: still open (latent).
- @antonwolfy, make ci-local crash, main-push concurrency, collect() and the docstring: fixed on head.
- napetrov, wheel data files (ci/test.py line 48), trailing-comment skip, FreeBSD classifier, CRLF METADATA: fixed on head.
- napetrov,
>3.10yields a 3.10 cell (ci/matrix.py line 162): still open (probes/out-p4_edges.txt). - napetrov, Windows and macOS cells on the Linux runner (intel-ci.yml line 47): still open, latent; conda cells are Linux only today.
Coverage: checked the diff, the ci/ readers on head with the probes above, the napetrov and bot threads against head, and the CI log lines against both trees. Not looked at: the full pytest suite (pytest plugins missing here, ran with the addopts cleared), conda and wheel builds (no conda here), ruff and mypy.
| if ( | ||
| package.is_file() | ||
| and package.name.endswith(PACKAGE_SUFFIXES) | ||
| and "broken" not in package.parts |
There was a problem hiding this comment.
nit: a build whose temp path has a directory named broken loses its package
ci/build.py:285 (collect): the filter tests package.parts, the whole absolute path, not the path under the scratch dir. A TMPDIR such as /x/broken drops every package, and the job fails with build: the build produced no package.
How to check: collect(root/"scratch", out) with a package under root/broken/ci-run/ → BuildError: the build produced no package (probes/out-p3_collect.txt; introduced, new file).
Expected: only packages under the scratch dir are filtered; a build under /x/broken keeps its package.
This is a different defect from the stale-package one in the same function (#378 (comment), fixed on head): here the filter itself is wrong.
Fix: test package.relative_to(scratch).parts, and add a test with a broken directory above the scratch dir.
| CONDA_SPEC_RE = re.compile( | ||
| r"3\.(\d+)(?:\.\*)?(?:\s+\*_(?:cpython|cp3(\d+)(t?)))?" | ||
| ) | ||
| CLASSIFIER_RE = re.compile(r"Programming Language :: Python :: 3\.(\d+)") |
There was a problem hiding this comment.
nit: a non-ASCII digit in a classifier is read as a Python version
ci/matrix.py:169 (CLASSIFIER_RE): \d matches any Unicode decimal digit and int() accepts it, so a classifier with a fullwidth 4 reads as 3.14 with no error. REQUIRES_PYTHON_RE uses \d the same way. The conda reader rejects the same digit (CONDA_SPEC_RE), so only the pyproject reader is silent.
How to check: from_pyproject with Programming Language :: Python :: 3.1４ → [Python(version='3.14')] (probes/out-p6_b2_b4.txt; introduced, new file).
Expected: a non-ASCII digit is refused, as the conda reader refuses it.
Fix: use [0-9] in CLASSIFIER_RE and REQUIRES_PYTHON_RE, with a test on a fullwidth digit.
| | `matrix.py` | Reads the platforms and Python versions from the recipe, or with `--system wheel` from `dokimi/pyproject.toml`. A pull request gets the oldest and the newest versions. | | ||
| | `build.py` | Builds one Python version: with conda-build, which runs the recipe's own tests, or with `--system wheel` with `pip wheel`. | | ||
| | `hooks.py` | The workflow's test jobs: installs one package into a clean environment with the requirements in `config.yml`, and runs `test.py`. A conda package's environment comes from conda-forge; a wheel's has only Python from conda-forge and everything else from PyPI, pinned by `dokimi/requirements.lock.txt` except on a release tag. | | ||
| | `config.yml` | Names the workflow that builds the packages, its artifact names, and what the tests need. | |
There was a problem hiding this comment.
nit: config.yml's workflow and artifacts keys are read by nothing
ci/README.md:18 (config.yml): the README says the file names the workflow and its artifact names. No code in ci/ reads those keys (hooks.py reads only the test key), and intel-ci.yml hard-codes the names, so editing them changes nothing, silently.
How to check: grep for readers of workflow and artifacts in ci/*.py → none (probes/out-b3.txt; introduced, new file).
Expected: the README describes only keys something reads.
Fix: remove the two keys and the README clause, or read the names from the file where the workflow uses them.
| if __name__ == "__main__": | ||
| try: | ||
| sys.exit(main()) | ||
| except DeclarationError as error: |
There was a problem hiding this comment.
nit: a non-UTF-8 pyproject.toml ends the matrix job with a bare traceback
ci/matrix.py:736 (main): it catches only DeclarationError, but from_pyproject lets UnicodeDecodeError escape, and invalid TOML raises TOMLDecodeError the same way. The job still exits 1, with a Python traceback instead of the matrix: message.
How to check: from_pyproject on a latin-1 pyproject.toml → UnicodeDecodeError (probes/out-b4.txt); invalid TOML → TOMLDecodeError (probes/out-p4_edges.txt; introduced, new file).
Expected: a DeclarationError naming the file, as for the other bad inputs.
Fix: decode and parse inside the reader and raise DeclarationError, with a test on a latin-1 file.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at ab77902. The delta since 21b51d5 holds up: the CRLF METADATA fix, the trailing-comment skip, the exact POSIX classifier match and the requires-python filter all give the right answers on the cases I tried. The merge still waits for one open point: exai-bot's report that the conda test jobs fail without mypy in ci/config.yml. One nit inline.
What I ran, on a Linux review host at ab77902:
make ci-local: 30 passed, 1 failed, 11 skipped; 12,964 tests. The one failed step isci.yml:test, with two timing assertions (test_unbalanced_quote_in_hook_completes_quickly: 1.19 s against a 1 s limit;test_the_regex_limit_is_shared_by_every_pattern_of_one_check). Neither file is touched by the PR, and both pass when run alone (2 passed), so I do not count them against it.- Probes of
with_requirements(LF, CRLF, no body, empty list) and ofadmits(>=3.11,<4,<3.15,!=3.12,~=3.12,~=3.12.1,>3.12.1,<=3.12.5, malformed clauses): all correct. Mutations of the new code: forcing LF, dropping the blank line, revertingcollect()to list the whole directory, emptying the POSIX exact list and addingwindowstoBUILT_PLATFORMSare each caught; the mutants named in the inline nit pass the fulltest_intel_ci.py.
Waiting on a fix: the conda test jobs install only what ci/config.yml test.requires lists. exai-bot's CI log at this head has Test Linux Python 3.11 and 3.14 failing on a typecheck test that needs mypy. That test (tests/test_repo_l0_typecheck.py) is not in the PR's base, so I could not reproduce the failure here. Today's main has it, and dokimi/requirements-test.txt there pins mypy==2.4.0 (line 15), so once this PR is merged into main the conda test jobs need mypy in test.requires.
I agree with @napetrov that the "broken" not in package.parts filter in ci/build.py:285 tests the whole absolute path. New evidence: deleting the filter leaves all 23 tests in test_intel_ci.py passing.
Open bot points I confirm:
- I agree with exai-bot that
ci/config.ymltest.requiresomits mypy, whichdokimi/pyproject.tomllists in its dev extra. New evidence: today'smainhastests/test_repo_l0_typecheck.pyand pins mypy inrequirements-test.txt, so the omission shows once this PR is merged into it.
Checked, no issues
- Commits: all 13 carry a
Signed-off-by. - Docs: the commands and flags in
ci/README.mdexist; the workflow is Linux only, asmatrix.pyand the README say; artifact names in the upload and download steps match. - Concurrency and collect(): the group equals
ci.yml's, andcollect()returns only the packages a run moved.
Bot points I checked that no longer apply at this head (5)
- exai-bot's point on the shared
concurrencygroup does not apply:intel-ci.ymluses ci.yml'spull_request.number || run_id, and a test pins the two equal. - exai-bot's point on stale packages in
collect()does not apply to what it reports:collect()returns only the packages this run moved, and every workflow build job writes to a fresh output directory. - Copilot's point on the
*.condaupload glob does not apply: conda-build from conda-forge writes.conda, andif-no-files-found: errorfails loudly on any other format. - Copilot's point on the module-level import in
test_conda_recipe.pydoes not apply:scripts/conda_forge_specs.pysits next totests/wherever the tests run. - Copilot's point on splitting entry points on
' = 'does not apply: the test compares withmeta.yaml, so another spelling fails loudly instead of passing.
| ("<3.13", [12, 13], "3.13"), | ||
| ("<=3.13", [13], "3.13"), | ||
| (">=3.10,!=3.12.*", [10, 12, 13], "3.12"), | ||
| ("~=3.12", [11, 12], "3.11"), |
There was a problem hiding this comment.
Nit: the ~= upper bound and the ==X.* branch of admits are not pinned.
Mutation check: replacing the ~= branch in ci/matrix.py with admitted = left >= right passes the full file (23 passed). So does replacing the ==/!= wildcard branch with one that only handles !=. The real function returns False for ~=3.12.1 on 3.13 and 3.14, and True for ==3.12.* on 3.12 only.
Suggestion: add two rows after line 107: ("~=3.12.1", [12, 13], "3.13"), and ("==3.12.*", [12, 13], "3.13"),.
main's typecheck tests run mypy (requirements-test.txt pins 2.4.0), and a conda test cell installs only the package and ci/config.yml's list, so both cells failed those tests. A test now checks the list against requirements-test.txt. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
build.py and matrix.py named MKL as the reason for wheel-requires and for x86_64-only macOS; dokimi neither uses MKL nor builds for macOS. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The pyproject table is named after this repo's own script, ci/build.py. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Summary
Adds Intel Internal CI (gh-ci):
ci/, a noarch conda-forge recipe inconda-recipe/, and.github/workflows/intel-ci.yml. The workflow builds the conda package and the pure-Python wheel for Python 3.11 to 3.14 on Linux and runsdokimi/testsagainst each installed package. Intel's internal CI then tests and scans the same packages.release.ymlstays the release process: gh-ci publishes nothing for dokimi.Changes
conda-recipe/:noarch: python, conda-forge only, Linux only.tests/test_conda_recipe.pykeeps its version, Python floor, console script and run requirements in step withpyproject.toml.ci/test.py: runs the suite with the project's pytest config against the installed package, and fails ifdokimiimports from the checkout. Two tests that need the source layout are deselected, and the installed package's data files are checked instead.ci/matrix.py,ci/build.py,ci/hooks.py: the shared gh-ci scripts, plus generic support for a project one directory down and a pure-Python wheel built withpip wheel.Risk
New files only; nothing existing changes. A release now also bumps the version in
conda-recipe/meta.yaml, whichtest_conda_recipe.pyenforces;docs/releasing.mddoesn't mention it yet.ci/is outside the ruff and mypy scope.Testing
On the fork, with a temporary push trigger: Intel CI built and tested all four conda packages and wheels, and ci passed apart from four
test_repo_guidance_history.pytests that need a git identity on GitHub-hosted runners. The changelog-fragment and comment-block checks pass locally.Revert
Revert the commit; nothing else is left behind.
Checklist
pytestpassesruff,mypy, and schema checks passAGENTS.md.github/.licenserc.yaml)