-
Notifications
You must be signed in to change notification settings - Fork 3
ci: Added a compatability matrix workflow to test agentune against lowest/highest supported runtime dependencies with various Python versions #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@shacharlevy1991 can you add a link to the result of running this that you showed me? |
@danarmak Hi Daniel, you can see the job(s) logs here. There are a total of 6 jobs inside the matrix: https://github.com/SparkBeyond/agentune/actions/runs/20332166207 |
danarmak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry about the delay in reviewing.
I think 'lowest version' isn't working. I'm not talking about the tests failing, I'm handling that in a separate ticket (SparkBeyond/aoa#410). I'm talking about the installed dependency versions.
For example, we declare in pyproject.toml a dep on numpy = "^2.2.4". In poetry.lock we have numpy at 2.3.4. And the latest available upstream version is 2.3.5. (2.4.0 was released on Dec 20th, so after you ran this.)
In the git action log for each 'lowest deps' variant, I see that it's installing numpy 2.3.4, the version in the logfile. And not 2.2.4, the oldest required version.
This happens when running poetry install --only dev, not when running uv. numpy is a main dependency, but some of the dev-only dependencies also depend on numpy. And we still need to get it down to its lowest supported version.
@danarmak Hi Daniel, indeed, it looks like numpy version 2.3.4 is installed since I did not know that Just for clarification, do we want to install the dev dependencies at their lowest versions as well? or is it just this specific case with |
|
We don't care which versions of the dev dependencies are installed. (And there can be other 'shared' dependencies besides numpy, now or in the future.) |
@danarmak There wasn’t a straightforward way to resolve this because we need both the lowest versions of the runtime dependencies and the development dependencies required to run pytest. As you mentioned, some dependencies are shared between the two sets. Since we don’t particularly care about the exact versions of the development dependencies, I created a bash script that parses the dev dependencies from pyproject.toml and generates a requirements.txt on-the-fly. This allows pip to install them without triggering unwanted upgrades — which was an issue when using Poetry, since it always resolves versions based on the lockfile. Check out the latest runs (in the checks of this PR), and let me know your thoughts. |
|
@shacharlevy1991 I really don't think we should use bash to parse text files. I can at least read this code because I used to write a lot of bash back in the day, but I'd still have trouble seriously trying to review this for bugs! A python script would be much simpler, I think. Besides being in python, it could use tomllib (which is in the standard library) instead of manually parsing strings. Side note: there's an inherent problem here. If a dev dependency transitively depends on a newer version of a main dep than the one we formally depend on, then the main dep will still be updated when we install the dev deps. This is unavoidable as long as we install the dev deps in the same venv, but it's not the real experience of a user installing our built package (and not the dev deps). I think we can ignore this at least for now, we don't actually have a problem of this type, but we should keep it in mind. |
@danarmak I wanted to avoid any script that would need to use the Python environment, hence picked Bash. I can try this with a python script instead. |
|
@shacharlevy1991 I just merged #134 which should solve the immediate problem that's making the lowest-version runs fail, can you rerun this? |
We do have python here, and this doesn't require any extra dependencies and should be a simple script that just works. It wouldn't be the end of the world to merge this bash script, since it's only for this job and can at worst break it, but I don't like the idea of having to change its text parsing when the pyproject.toml structure changes a bit six months from now. |
|
@danarmak Most tests except lowest_deps on 3.14 work after introducing the new Python script and your fixes. The jpb failes for the following reason based on ChatGPT: What’s happening: You are trying to install pyarrow>=19.0 (lowest allowed by your dev dependencies) on Python 3.14. pyarrow has native C++ extensions and requires Arrow C++ libraries or cmake to build from source if no compatible wheel is available. For very old versions like 19.0.1, there are no prebuilt wheels for Python 3.14, because Python 3.14 is new. So pip attempts to build from source, which triggers: Setuptools warnings (deprecated license, file patterns, etc.) CMake errors because the Arrow C++ SDK is missing. Essentially: Python 3.14 + pyarrow 19.0.1 → cannot install via pip unless you build Arrow manually. |
… preform upgrades
8cfd310 to
10351ab
Compare
|
@shacharlevy1991 this looks much better! The reason the run on 3.14 with lowest dependencies failed is because pyarrow 19 doesn't support python 3.14; we need pyarrow >= 22 in that case. Pyarrow itself declares this, but your script successfully forces pip into trying to install pyarrow 19 on python 3.14, which predictably fails to compile. To declare in pyproject.toml different minimal version requirements for the same dependency on different python versions, we need to write this: And your script would have to support that. This quickly becomes unmaintainable. I was hoping there would be a way to call existing tools to do the job, without creating a requirements file manually. Do you have suggestions? To be clear, this PR is already very useful. Getting notice that the newest released version of a library breaks our tests is much more important, and more frequent, than finding out that the oldest version we support doesn't work. So even if that's all we accomplish in the end, I'd still want to merge that part. |
@danarmak If by "script" you mean the Python script that parses |
You're right. This is not a problem that would ever happen with dev deps, because we can always update the required min version to be as high as we want. Then I think we can merge this. And make the change to the pyarrow dep either here or in another PR. Am I forgetting anything? |
@danarmak I'm fine with that, just note it cannot find a wheel for scipy as well: I think we now have a simiilar issue with scipy: https://github.com/SparkBeyond/agentune/actions/runs/20487686301/job/58873345363?pr=132 After you approve, I'll just need to make a small change so it won't run for PRs automatically. |
|
@shacharlevy1991 I see you're still changing the parsing script; let me know when you've pushed the final version so I can review and hopefully approve it. |
@danarmak have done with all changes. Once you approve I just need to remove the pull_request trigger |
danarmak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
The script doesn't need to be perfect and we can fix it if our project file ever evolves to be more complex than it can currently handle.
I'll fix the scipy dep and any other similar problem in a followup PR.
removed ubuntu updates and pull request trigger
What does this PR do?
This PR add a compatibility matrix workflow that does the following:
poetryis used to install the development dependencies only.uvis used to wrap pip and allows the usage of thelowest-directandhighestflag. It uses thepyproject.tomlfile as a reference to determine the lowest/highest version supported by our project (not to confuse with the latest available on PyPi).poetryagain is used to run the tests.Notes
python-version: ["3.12", "3.13", "3.14"]input to ensure the latest available patch version of python is used for each run.Caveat: “latest” depends on what versions are available in the runner’s environment. For GitHub-hosted runners, they maintain a set of pre-installed Python versions; if a newer patch is released after the runner image was built, it may not yet be available until the runner image is updated.
Related Issues
Fixes SparkBeyond/aoa#338