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

Functionality to run things on the remote server #72

Merged
merged 5 commits into from
May 26, 2022
Merged

Functionality to run things on the remote server #72

merged 5 commits into from
May 26, 2022

Conversation

haz
Copy link
Contributor

@haz haz commented May 10, 2022

Closes #71

@haz haz changed the title Closes #71 Functionality to run things on the remote server May 10, 2022
@haz haz marked this pull request as draft May 10, 2022 18:32
@haz haz marked this pull request as ready for review May 18, 2022 03:51
@haz haz requested review from YIDING-CODER and nirlipo May 18, 2022 03:52
@haz
Copy link
Contributor Author

haz commented May 18, 2022

@YIDING-CODER / @nirlipo : Could one of you give this a spin?

  1. Install this version of planutls (and set it up with planutils setup).
  2. Run things that would be accessible on the remote server. E.g.,
planutils remote lama-first domain.pddl problem.pddl

It works for me, but I'd like someone else to kick the wheels a bit (different services, different ways it may break, etc).

@haz
Copy link
Contributor Author

haz commented May 24, 2022

@YIDING-CODER / @nirlipo : Any movement? Would like to have this and another major issue done and in place before ICAPS...

@nirlipo
Copy link
Collaborator

nirlipo commented May 25, 2022

I'll get back to you in a few hours


args = {arg['name']: arg for arg in remote_package['endpoint']['services']['solve']['args']}

if len(options) != len(call_parts)-1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to compare with the number of arguments, not the number of call parts, as some manifests include predefined call arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried to push a commit fixing this line but I get the following error:

$ git push
remote: Permission to AI-Planning/planutils.git denied to nirlipo.
fatal: unable to access 'https://github.com/AI-Planning/planutils.git/': The requested URL returned error: 403

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Permissions fixed.

But I think the comparison is correct. options is the command-line options, and call_parts are what will be called on the remote. They should match the # of arguments, but there's no guarantee (it's up to the manifest maintainer). Here, we assume that the local call (filling out options) will match precisely the remote call.

Copy link
Collaborator

@nirlipo nirlipo May 25, 2022

Choose a reason for hiding this comment

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

Thanks! Yes, but it should check against number of args instead. See line 228, we iterate only over the arguments of the call defined in the manifest by looking for the { and } symbols. I'll make the commit so you can test it. see 30a8426

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, I see the change, but I was just trying to align the command-line call with the remote call string -- should mirror closely, as otherwise there is no way to align. I get that this fixes the ... plan issue, but I think that may be more of an issue with the package -- should be able to specify the plan name. A more direct regex would be better, but meh...I think this is good enough for now.

#75

Copy link
Collaborator

@nirlipo nirlipo left a comment

Choose a reason for hiding this comment

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

Works like a charm for lama-first. I tried several domains, some unsolvable and the process ended after 20s.

I got the solution as a file in my local env.

The only message which may be confusing for the user is the {stderr}:


        {stderr}

bash: cannot set terminal process group (1): Inappropriate ioctl for device
bash: no job control in this shell
bash: cannot set terminal process group (1): Inappropriate ioctl for device
bash: no job control in this shell

I tested the other packages, e.g. planutils remote dual-bfws-ffparser domain.pddl problem.pddl but I get the error message:

Call string does not match the remote call: dual-bfws-ffparser {domain} {problem} plan

I then add plan or foo as an extra argument, and it works, but regardless, it always returns a file named plan, even if I pass foo.

Same issue happens with delfi.

Looking into the manifests of delfi, lama and bfws, we can see that all of them expose 2 arguments through the planner template manifest, as pointed out by the json file catched by the package API, but they have a different call argument, adding a dummy plan at the end. I sent a commit fixing this issue.

I think we need to add a remote list comand, that prints the json files names returned by the packages API: http://45.113.232.43:5001/package

This would emulated the functionality of planutils list, that shows which packages are installed.

@haz
Copy link
Contributor Author

haz commented May 25, 2022

Nice!

The only message which may be confusing for the user is the {stderr}

This is what's being spit out on stderr on the remote server. Maybe an issue for PaaS?

I then add plan or foo as an extra argument, and it works, but regardless, it always returns a file named plan, even if I pass foo.

Ya, that's a finicky one. Maybe the length check should be re-thought? Issue is we don't have key-value stuff coming in...just arguments on a command line that we're trying to align with the remote call string.

Love the idea of a remote-list!

@nirlipo
Copy link
Collaborator

nirlipo commented May 25, 2022

Done! Remote List feature added, see 70b49bd

Copy link
Collaborator

@nirlipo nirlipo left a comment

Choose a reason for hiding this comment

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

Ready to be merged into the main branch.

@YIDING-CODER
Copy link
Collaborator

@haz @nirlipo Sorry, I am having lots going on recently, and I missed the email before. I saw Nir has made some progress on it during the day. Is there anything left that need to be fixed or developed for the "planutils remote" feature?

@nirlipo
Copy link
Collaborator

nirlipo commented May 25, 2022

No problem @YIDING-CODER ! Any other command-line argument that we should add given the existing functionality/API provided by the new solver online service?

We have exposed so far the solve (remote) endpoint and we list the remote packages installed (remote-list).

If we can't think now of other command-line options, then, it's ready to merge from my end.

@haz
Copy link
Contributor Author

haz commented May 26, 2022

I'll merge now, but @YIDING-CODER , you could take a look at PaaS to see where this might be popping up (even on calls that are ok):

        {stderr}

bash: cannot set terminal process group (1): Inappropriate ioctl for device
bash: no job control in this shell
bash: cannot set terminal process group (1): Inappropriate ioctl for device
bash: no job control in this shell

@haz haz merged commit 2ab7015 into main May 26, 2022
@YIDING-CODER
Copy link
Collaborator

@haz,
I noticed this bash things start poping up after we release the new docker image. I think it is not caused by the Paas serivce, but the docker environment. When I run the planners inside the docker contatiner, this bash message came up. I will look into it soon.

@haz
Copy link
Contributor Author

haz commented May 27, 2022

You mean planutils docker image? There are many images at play ;).

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.

Remote mirror
3 participants