-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add cibuildwheel support #448
Add cibuildwheel support #448
Conversation
(Converting to draft while I try to figure out Windows builds) |
This is very exciting! Thank you so much for looking into this. I would love for users to be able to simply
For future work (it's a bigger change), we may want this to be usable for general users that want to run VisualSoar, the debugger, etc. from the launch scripts. We would need to include the scripts/jars for running these into the wheel, and we would need to notify the user of where Soar was installed (previous point). Right now the final Soar distribution is built using a separate project: https://github.com/SoarGroup/Release-Support. Regarding more Pythonic access for Soar, there is a popular (in our tiny community) project that does something similar called |
Already intending to, so that
Not necessary, as Soar gets built and statically linked against the SML interface library (or at the very least, included in the wheel), meaning that its essentially bundling a distribution of Soar with the wheel itself as well.
Well, multiple reasons;
So part technical reason and social/cleanliness reason. There could possibly be an extra package which installs that
I... would have to take a closer look at that, at the moment I'm really just focusing on getting the SML part up and ready. I don't have any thoughts on this at the moment, and the decision on wether to bundle the VisualSoar parts (if in includes java; no), making it easy to make VisualSoar/debugger connectable (probably), or something else, is something I don't have enough info for. This library is just intended to start the soar kernel, load agent code, and setup the IO link via python. Debugging and whatnot should then be pluggable similar to how someone who usually runs Soar would expect it to work, imo. For visualsoar or the debugger to connect, the programmer would have to first fire off python with a script that loads the soar agent (with its connector), and only then they could connect to it, methinks. This would miss out the connector part of debugging, which would need to be done on the python side in the IDE's native debugger.
I saw, though thanks for mentioning it for if I hadn't :) I'm going to take a look at it for inspiration, but largely the reason I want to make an independent library is because I then both learn the internals of SML while thinking of a pythonic interface, hopefully with that fresh perspective not being bogged down by rigidity to conform to something similar to Soar's internals, etc. Taking a look again, the sub-classable connector interface does strike a fancy; I think I'd like more parts of that to be as modular, such as entire input or output trees, "interpreters", so that data gets transformed into WM elements efficiently, and such interpreters/trees can be modularised and shared around, iterated. (plus having nice formatted value trees on That library design looks poll-based instead of push-based; I want to play around with that as well, where instead of the WM being polled for every update, the programmer can opt to instead explicitly push data at "off" intervals (decoupled from the decision cycle). But I have to think how such a library design would work. One nice thing about decoupling this from the "raw" library is that I can do alpha/beta versioning while I experiment with this :) Edit: An additional thought; I can make it work with asyncio, instead of requiring synchronous code, which would then help introduce tons of libraries which do asynchronous IO off the device (such as driving motors, or APIs) |
a979dec
to
d1b44b4
Compare
What do you mean by this? Do you mean that you plan for the import to be from something more nested, like BTW I think calling it I think we can punt the question of more general usage down the road for now. Having any form of pip-installable Soar would already be very cool. This is also related to the point where you say
A nice role-reversal! Give highest-level control to the external application instead of to Soar. This programming pattern is less common for Soar because it is more complex, but I would love to see it done. I could be mistaken, but I think you'd want to create the kernel in the current thread using I think your ideas for a Pythonic Soar package sound really cool, so I'm excited to see what you come up with! |
Felt a bit inspired and started drawing up the different ways that soar and the environment could asynchronously run alongside eachother, apologies for the digression. (Click to open)I thought about how Here's a simple loop; Soar (grey) runs one decision cycle, and then waits before the output (red) has been completed on the connector, then waits before the input (blue) is completed on the connector, and then runs another cycle. However, due to asyncio, it is possible to say "Hey, run the output code together with the input code, and only wait for the input to complete"; This would have the input and output code fight for the state of the world, but this may be possible in some scenarios (or some sub-connectors), and a programmer might want to enable this for optimisation purposes. Then there's a final method, which runs the input collector every time, and then feeds soar the last result (encircled), while also asynchronously running the output; Again, this would have implications, but could be something the programmer wants to weigh off. All of these have tradeoffs, but supporting all of them is possible, and its good to provide options. |
Currently, the import would be
That's fair, then I think I'll indeed swap it around for that (making
To be honest, I think pivoting to packaging the python binding for soar to a wheel file for users to install would be more user-friendly in the long-term, since instructing them to manually pivot
I'll have to see what is most optimal, what I'm seeing here is that doing |
All sounds good :) One last note on concurrency: core Soar does not support it. It is not thread-safe. The SML client library uses locking to ensure that only one command can get sent at a time, and on the kernel side there is a queue for receiving commands over the network socket (as when the debugger connects). So there are a couple safety measures in place, but if you're not careful you can run into issues like #430. This is also related to the actions one might take in a handler; when control returns to Soar after a handler is complete, it doesn't do any checking to make sure that, e.g. the agent list hasn't changed. Unfortunately these edge cases aren't well-documented, but if you stick to actions that were definitely intended and designed for, such as creating WME's on the input link in the input handler and reading command WME's on the output link in the output handler, everything will work fine. |
6b761ad
to
d1b44b4
Compare
e6d393f
to
6b64901
Compare
O A little bit off topic: I am currently working on a Soar ROS2 integration and I implemented something similar to your first idea but I instantly push the elements to a consumer thread via queues so Soar remains responsive for output link wmes. For the input I read all queues and attach the data to the input link. I did not publish the code (cop) yet. So far this approach works great and I did not experience any weird behaviour. I hope this helps! |
CI builds properly now, so there are a few outstanding tasks, and some notes:
Notes:
|
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.
Added a few notes. These can all be resolved, but just a few thoughts wrt points in the changes that I think should be noted.
SConstruct
Outdated
try: | ||
enscons_active = True | ||
import pytoml as toml | ||
import enscons, enscons.cpyext | ||
except ImportError: | ||
enscons_active = False | ||
|
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.
This makes sure that even if someone is building it without cibuildwheel+enscons, this script can still run normally.
You've given us a lot here. We need to review a few of these things (I haven't caught up on all the stuff that's happened here), but I like this so far. Thanks! You say "Installing this would instantly recover all compatibility with all the existing projects, and make them properly portable as a result :)" "so just say 'alright' to that if I should do so in this PR" Alright. "I've uploaded the current build artifacts to pypi.org/project/soar-sml, this is to prevent someone else nicking and squatting it (I've had that happen a few times before)" --Thanks for that, too! |
Alright, I added It is now possible to do |
I created a garfieldnate account and a SoarGroup organization on pypi today; the organization has to be approved by someone before it exists, so I'll let you know when that comes through.
Most of the actual running code is going to be compiled C++, and I'm not worried about the speed of the wrapper code. Do you think I should be? Soar does a lot of work, and I would hope that by comparison a simple method call in Python would be no big deal.
I think starting with 9.6.2 is fine. Going back and building earlier versions of Soar is a big pain.
That's fine, we don't support it anymore.
Definitely only need the latter, but I wouldn't say it's a show-stopper for now.
How does this work? The shard library that Python loads only works with 3.12, which is the version of Python that we build with. This would mean that the wheel wouldn't work at all, since it can only be used with versions of Python that the lib won't work with, but you said it's working fine, and I don't understand how.
I would have thought it were complete magic if this worked for everyone :D It takes a bit of work to set up the environment to build Soar. I think it's fine to go without this. |
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.
This is looking great! I'm really excited. Most requested changes are just documentation/comments.
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.
Beautiful! Backwards-compatibility. Thank you!
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 really happy about how thorough you're being!
...right, I see what's wrong, you're on MacOS 13, while the builder aims for MacOS 14, and so there are no wheels it can download. This should be configurable, and I'll attempt to get it down to MacOS 12 or lower, to maximise compatibility with old MacOS versions.
|
@garfieldnate I've fixed the problem with the latest commit, this should now properly "downversion" the built-for mac wheels, so that it tags itself by the minimum mac version it can get downloaded by, and as a result we got Intel Mac compatibility for Mavericks (10.9!), and all Apple Silicon Macs :) I've uploaded the new build artifacts that tag these correctly under (Excluding python 3.8 on Apple Silicon (ARM64) macs, for reasons I poked earlier, and in the file comments) |
I tried it just now and it just worked! Tested on my ARM Mac, then x86_64 Ubuntu and Windows 11. |
The way I'll be doing it (by splitting the jobs) will make CI faster, too. I'll add a CI job that'll publish the build versions to test.pypi.org, and I'll add a CI job that'll publish them to regular pypi if the CI run gets triggered by a tag push / release. |
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.
- Added a dynamic versioning mechanism, which will:
- For development and local versions, version according to the offset of the latest release tag, and add extra metadata.
- For release versions, version according to the tag of the current commit.
- Seperated out the wheel building into seperate jobs, which concurrently run faster.
- Moved documentation for developing and packaging to its separate file.
- Added an uploading step:
- Dev builds on a schedule, every week, uploaded to https://test.pypi.org/p/soar-sml
- Final versions on GitHub Releases, uploaded to https://pypi.org/p/soar-sml
A development version can be installed via this command now:
pip install --pre -i https://test.pypi.org/simple/ soar-sml
I'm having some trouble understanding the GitHub UI, but as far as I can tell, the one question I just posted is the last unresolved conversation, correct? And then it's ready to merge, right? |
One other question 😅 Do we need to upload soar-compat to test.pypi.org? |
Yes
If you feel like it is, yes :)
...Yes, actually, I just tested it, and it can't find soar-compat, so I'll upload it right now ^^; Edit: done :) |
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.
Thanks for all the hard work! I look forward to you sharing this at the workshop! 🎉
Thank you a lot! 💚 |
This PR adds cibuildwheel support for python wheels: allowing for CI-powered reproducible python builds into self-contained wheels.
(Python wheels are distribution packages containing everything a python library needs to run, including shared libraries. They're tagged with compatibility markers (OS, Architecture, and for linux: GLIB version), so that
pip
can download the correct variant from a package index like https://pypi.org)This is achieved with
enscons
, which puppetsscons
to achieve the right builds and environments.This names the output library as
soar-sml
, and renamesPython_sml_ClientInterface.py
to__init__.py
, so thatimport soar_sml
will have the same effect asimport Python_sml_ClientInterface
.As this then becomes a
pip
-installable library,sys.path.append
is not required anymore,import soar_sml
will do the trick from anywhere on the system.TODOs
.github/workflows
file for doingcibuildwheel
pyproject.toml
authors
,keywords
,classifiers
,urls
, and probably another few fields in thepyproject.toml
file needs to be updated/added with the right data.My goal with this is to make Soar ready to be packaged for https://pypi.org distribution (likely in a future PR, with proper discussion), and to then create a library that will make accessing soar even more pythonic. (Name pending, also, soar was taken,
*shakes fist*)I hope that both of these efforts will lower the barrier for entry and experimentation with Soar, especially as SML allows anyone to create a "body" for Soar agents to "interact" with the world.