aig-upf / tarski

Tarski - An AI Planning Modeling Framework
Apache License 2.0
59 stars 19 forks source link

Switch to clingo pypi package #123

Closed anubhav-cs closed 2 years ago

anubhav-cs commented 2 years ago

Alters the clingo_wrapper, removing the requirement to manually install clingo(gringo) binary.

miquelramirez commented 2 years ago

Hi @anubhav-cs,

thanks for this. I am merging into aig-upf:devel.

Miquel.

haz commented 2 years ago

What was the motivation behind this? I'm worried that we may face issues wrapping this up for in-browser use...

miquelramirez commented 2 years ago

What says on the tin

removing the requirement to manually install clingo(gringo) binary.

haz commented 2 years ago

Aye, but just out of convenience? It the same clingo binary that ships via pip? If so, why not just locate it differently and have a two-line change to nab the path right and include in the requirements? I think I'm missing some broader consequence here. Any new python package requires rebuilding that dependency (and following the chain of what it requires), so potentially a major setback for browser embeds.

miquelramirez commented 2 years ago

It the same clingo binary that ships via pip?

Unless some third party is impersonating the Pypi repository, I would assume so.

If so, why not just locate it differently and have a two-line change to nab the path right and include in the requirements?

Because it is not the (de facto) standard way of handling packages in Python. All the PEPs and recommendations I see only strongly suggest to use Pypi whenever available over alternative methods.

I think I'm missing some broader consequence here.

If I were you I would consider the possibility that there is no such consequence.

Any new python package requires rebuilding that dependency (and following the chain of what it requires), so potentially a major setback for browser embeds.

IIRC the "tarski in the browser" use case 1) Windows-only and 2) depending heavily on Anaconda? Is that correct?

In any case, the gringo dependency should probably an extra dependency (e.g. you only get it if you run the command pip install tarski[gringo]). That's a very simple change to request with exactly 0 drama attached to it.

haz commented 2 years ago

Nah, not the use-case and it (gringo) is actually something we use (for grounding). Concretely, being able to get it all going in the browser is what enabled this award-winning plugin, and opens the door to smart grounding client-side in the browser for anyone using the online editor.

miquelramirez commented 2 years ago

So little drama that here is the change 1d4ab534c613035d3e63f60f04e648d5f6e932a3

If there's a subset of tarski dependencies which should be made optional to support delicate environments @haz, please make a list and a RFC. Making a PR with the "optimal" dependency package is also helpful (but of course, I retain the right to reject or amend the PR as it suits).

@anubhav-cs could you please check that the pip install tarski[gringo] command is working as expected on your development environment (it should, see docs here).

haz commented 2 years ago

Still doesn't help...the option is required in this case :P

miquelramirez commented 2 years ago

Then there you have your new default way of installing tarski if the Pypi solution suits. Nobody stops you or anybody from installing tarski without Pypi gringo, and then installing gringo by hand. Having it protected via the extra_requires method "shields" any award winning plugins from getting into trouble.

haz commented 2 years ago

The usage has fundamentally changed (e.g., here), no? Are you suggesting that a non-gringo tarski with gringo binary added manually should "just work" after this merge?

anubhav-cs commented 2 years ago

@anubhav-cs could you please check that the pip install tarski[gringo] command is working as expected on your development environment (it should, see docs here).

@miquelramirez Yes it works.

miquelramirez commented 2 years ago

Well, that's an entirely different matter @haz.

@anubhav-cs you will need to add as a fallback the old which-based method. My suggestion is that you have a method/function called get_gringo_command that creates the arguments to pass to cmd.execute. Just send another PR towards devel. Thank you.

anubhav-cs commented 2 years ago

@miquelramirez Easy peasy, will create a pull request soon.

haz commented 2 years ago

Merci both!

We could have potentially just frozen the version we include, but that's no fun...means all the latest/greatest stays out of the browser and editor.

anubhav-cs commented 2 years ago

I have created the PR https://github.com/aig-upf/tarski/pull/124 with the fallback method.