Skip to content
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

Switch to clingo pypi package #123

Merged
merged 1 commit into from
Feb 6, 2022
Merged

Conversation

anubhav-cs
Copy link
Contributor

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

…depending on manual installation of clingo(gringo) binary
@miquelramirez miquelramirez changed the base branch from master to devel February 6, 2022 21:21
@miquelramirez
Copy link
Member

Hi @anubhav-cs,

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

Miquel.

@miquelramirez miquelramirez merged commit 8be09d7 into aig-upf:devel Feb 6, 2022
@haz
Copy link

haz commented Feb 7, 2022

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

@miquelramirez
Copy link
Member

What says on the tin

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

@haz
Copy link

haz commented Feb 7, 2022

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
Copy link
Member

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
Copy link

haz commented Feb 7, 2022

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
Copy link
Member

So little drama that here is the change 1d4ab53

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
Copy link

haz commented Feb 7, 2022

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

@miquelramirez
Copy link
Member

miquelramirez commented Feb 7, 2022

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
Copy link

haz commented Feb 7, 2022

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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

@haz
Copy link

haz commented Feb 7, 2022

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
Copy link
Contributor Author

I have created the PR #124 with the fallback method.

def __init__(self, name):
self.app_name = name

def main(self, ctl, files):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Innocent question: do we need all this code if we're just simply using the default behavior of clingo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same question. Going by the documentation this seems to be the recommended way to get the default clingo app behavior https://potassco.org/clingo/python-api/current/clingo/application.html

Also, I checked it. The pip package doesn't include the gringo binary, instead it has a platform specific library with a python wrapper. So, we cannot nab the path to the binary.

"""
class WrapperClingo(Application):
def __init__(self, name):
self.app_name = name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this attribute being used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That attribute should be program_name. That's a typo on my part. It is a member of the Application class.

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.

4 participants