Skip to content

[IMPORTANT] Python workflow update#1537

Open
alexunderch wants to merge 27 commits into
google-deepmind:masterfrom
alexunderch:build-fix
Open

[IMPORTANT] Python workflow update#1537
alexunderch wants to merge 27 commits into
google-deepmind:masterfrom
alexunderch:build-fix

Conversation

@alexunderch

Copy link
Copy Markdown
Contributor

To whom it may concern:

While testing windows build, I've found out that there is now pyproject.toml with some interesting set of "full" dependecies:

full = [

It's not very useful as the main installation is done through setup.py. To combat the issue, I decided to basically "fork" #1071 because the author is busy or what. Therefore, I moved the building process to pyproject, when setup.py is just responsible for the CMake plugin

Overall this PR introduces:

  • We'll rid of python_extra_deps.sh and will have all deps directly installable, like pip install open-spiel[pytorch, jax]
  • We introduce explicit pylinter and pyink formatter following the google's rules as a pre-commit config
  • We can allow running tests and contributing from all platforms because the hooks automatically fix line endings and such
  • Adding a workflow to run formatters upon the contributed code
  • Modify the documentation

@lanctot, can you please, check out what what linter do you have because Google's basic linter raises a ton of docstring warnings/convention errors.

@lanctot

lanctot commented May 4, 2026

Copy link
Copy Markdown
Collaborator

@alexunderch properly moving to pyproject.toml is a big change that I don't have proper time to investigate fully at the moment (so likely want to delay until after OpenSpiel 2.0 is released).

But I agree: that list is too large. It should only contain the dependencies listed in requirements.txt.

In the short-term (separately, in a different PR), can we just reduce that list to just those packages in requirements.txt so that people are not downloading unnecessary extra packages?

@alexunderch

alexunderch commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

@lanctot technically, we already did this:
pip install open-spiel installs only packages in the requirement.txt,
pip install open-spiel[dev] adds pre-commit packages and linter, and so on.

I will fix some bugs. Maybe we can get optional deps + basic pre-commit (just linter + pyink) to the 2.0 release?
Just want to stop all that manual linter stuff that takes a month to prepare a PR. It irks me.

I think, then, I will take my time to cook it properly.

@alexunderch

Copy link
Copy Markdown
Contributor Author

Okay, the tests pass, but the setup takes significantly longer than before. I'll do some housekeeping.

@alexunderch

Copy link
Copy Markdown
Contributor Author

@lanctot, overall the new system works and it's a little bit faster and easier than just using setup.py. However, in some places, we don't need to build the whole package, just requirements, which slows down the tests for some platforms, for example, arm.

I still think to get rid of the python extra deps script with the env variables, because it won't really work on Windows, I reckon. Instead, I may create several requirements.ini files for diff modules. Wdyt? Now I use external pip-tools which is an external dependency, unfortunately.

Second of all, wdyt about introducing ninja, an external building system for Linux package? I found there it's might be used some of CUDA stuff in the library.

And lastly, can you comment someting on what's the linter approach on doc-strings should be?

Overall, the goal of the PR is to introduce (I don't really do much more than moving couple of lines of code around) a correct installing system that should allow to build the library from source on MacOS, Linux, and Windows, outline the issues for the latter. Moreover, for Linux and MacOS, we'll make some nice adjustments to lint and format the code automatically to lessen the burden of reviewing process.

I believe that it would be a nice addtion to the 2.0 release. Didn't want to leave the gap with the weird pyproject.toml and some incosistencies caused by Windows' build introduction.

@alexunderch

alexunderch commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

@lanctot, technically the most of PR is done, at least is what could be covered by tests:

  1. There's now no requirements.txt anymore, everything runs through pyproject.toml. Requirements are still used for the build, which now takes the same amount of time then before, but they are now automatically pre-generated by a simple python script. setup.py is only used for the CMake plug-in, we can't rid of that
  2. I cleaned up some mess introduced by windows preview.
  3. I tried to introduce basic (cmake+ctest tests) for the windows build, but they require some tweaking of ctest config and such, which I am not ready to play with because I do not develop for windows. But at least the package is installed through a venv like the other OS. (p.s. windows can run bash scripts no problem, but there are edgecases becayse Microsoft C++ compliler decided to be different!)

There is still a part with a linter. Because I turned on doc-string support of the publicy avaliable linter and there are so many warnings caused by the legacy code base, so I'wll repeatedly ask you to give it a look. As soon as you're ready, I can add a workflow that formats the code and runs the linter on the commited changes 😄 . This would be the final thing that this harmless PR introduces.

Have a nice day

@alexunderch

Copy link
Copy Markdown
Contributor Author

Let's say, this PR is review-ready

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.

2 participants