Skip to content

Fix PR #17 review items: PyCall env, codecov, compat bounds#22

Open
utensil wants to merge 2 commits intomasterfrom
fix/17-followup-review
Open

Fix PR #17 review items: PyCall env, codecov, compat bounds#22
utensil wants to merge 2 commits intomasterfrom
fix/17-followup-review

Conversation

@utensil
Copy link
Copy Markdown
Member

@utensil utensil commented Apr 3, 2026

Follow-up to #17 per review comments.

  1. PyCall Python env (HIGH): Added Configure PyCall to use system Python step so PyCall picks up the pip-installed galgebra==0.6.0 rather than falling back to its own Conda env with an unpinned version.

  2. codecov file (MEDIUM): Explicitly pass files: coverage-lcov.info to codecov-action@v4 instead of relying on auto-detection.

  3. compat bounds (MEDIUM): julia = "1.10" -> julia = "1.10, 1.11" to match the CI matrix explicitly.

utensil added 2 commits April 3, 2026 11:00
1. Point PyCall at system Python so it uses the pip-installed galgebra
   rather than falling back to its own Conda environment
2. Explicitly pass coverage-lcov.info to codecov-action v4
3. Set julia compat to "1.10, 1.11" to match the CI matrix
The separate Pkg.build("PyCall") step failed because the manifest
wasn't instantiated yet. Instead, set PYTHON env var on the Test step
so PyCall picks up the system Python during Pkg.build("GAlgebra").
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.43%. Comparing base (5b9a101) to head (9f313df).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #22   +/-   ##
=======================================
  Coverage   80.43%   80.43%           
=======================================
  Files           4        4           
  Lines          92       92           
=======================================
  Hits           74       74           
  Misses         18       18           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lodyai
Copy link
Copy Markdown
Contributor

lodyai bot commented Apr 3, 2026

Review — 3 issues + design consistency concern

1. PYTHON env var arrives too late (HIGH)

The PYTHON env is set on the "Test" step, but PyCall's Python choice is locked when Pkg.build("PyCall") runs — which happens inside Pkg.instantiate() on the line immediately before. By the time Pkg.build("GAlgebra") runs, PyCall is already built.

Fix: add an explicit step before "Test":

- name: Configure PyCall to use system Python
  run: julia --project=@. -e 'import Pkg; ENV["PYTHON"] = "${{ env.pythonLocation }}/bin/python3"; Pkg.build("PyCall")'

2. deps/build.jl fallback is still unpinned (HIGH)

The pip fallback for end users remains:

run(PyCall.python_cmd(`-m pip install galgebra --user`))

This installs whatever latest galgebra PyPI serves — not 0.6.0. The CI pin doesn't protect user installs. Should be:

run(PyCall.python_cmd(`-m pip install galgebra==0.6.0 --user`))

3. Design inconsistency between CI and runtime (HIGH)

CI now installs galgebra via pip into the system Python, and the PYTHON env fix routes PyCall to that Python. But __init__() uses pyimport_conda throughout:

copy!(galgebra, PyCall.pyimport_conda("galgebra", "galgebra"))

pyimport_conda will try to conda install galgebra into PyCall's Conda env if the import fails — bypassing the system Python and installing at an unpinned version. This creates two separate installation paths that can diverge silently.

The package needs a coherent design decision:

  • Conda-first: remove the pip install galgebra==0.6.0 CI step, instead use conda install galgebra==0.6.0 or add it to the Conda environment; pyimport_conda in __init__() stays.
  • pip/system Python-first: keep the CI pip step; replace pyimport_conda in __init__() with plain pyimport; update deps/build.jl to install via PyCall.python_cmd with a pin.

Right now CI and runtime are pulling in different directions — pyimport_conda was a deliberate design choice for the original Conda-backed setup, but the CI fix in PR #17 moved toward pip/system Python without updating the runtime side.


4. julia = "1.10, 1.11" is semantically redundant (informational)

In Julia compat syntax, "1.10" already means [1.10.0, 2.0.0). The union "1.10, 1.11" expands to [1.10, 2.0) ∪ [1.11, 2.0) = [1.10, 2.0) — identical. Julia 1.12+ will still be allowed. No action needed, just noting it doesn't restrict the range as it may appear to.

lodyai bot pushed a commit that referenced this pull request Apr 3, 2026
- deps/build.jl: detect existing galgebra, check version range [0.6.0, 0.7.0),
  install controlled version only when needed
- src/GAlgebra.jl: replace pyimport_conda with pyimport, add GALGEBRA_DEBUG
  env var for diagnostics
- ci.yml: explicit pip install galgebra==0.6.0, PYTHON env var for PyCall,
  bump Actions (checkout v4, setup-python v5, setup-julia v2, codecov v4)
- README: add Python dependency and diagnostics documentation

Supersedes #22.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant