robotpt/cordial

A framework for coordinating robot behavior.

cordial's Issues

Make GUI use actions instead of services

Currently, the GUI uses services, which are blocking until a request is recieved. It would be much nicer to use actions, which can be canceled.

Also, this would allow the timeout logic to be moved from the GUI to be within the manager, which is probably better design.

Unify how parameters are accessed in classes

Currently, nodes take in parameters in several different ways. It would be nice to unify how this is done.

I think it would be good to assign parameters to variables in class constructors, this would be instead of reading the parameters outside of the constructor and passing them into the constructor, which is much more verbose.

[CI] Run tests repeatedly to find flaky tests during the night

It would be great to run the tests repeatedly for a couple hours at night. This will help us to identify flaky tests.

[cordial_gui] Add low vision interface

We should make a GUI interface display for people with low vision. This means big text and high contrast.

Fix CI after Github Actions update

Github seems to have disabled the set-env command, we'll need to update our CI to avoid using this command.

Errors in Cordial GUI Example

Here is the error I found:

Traceback (most recent call last):
  File "example_say_and_ask_on_gui.py", line 4, in <module>
    from cordial_msgs.srv import Ask, AskRequest
ImportError: cannot import name Ask

Make cordial package a meta package

A good example is here.

Move GUI controller logic into the manager

It makes sense to move the GUI's controller into the manager because @emilyxzhou has updated the manager to use ROS actions, giving the manager the ability to cancel requests and update its plan based on feedback from the GUI (such as if the user is interacting with the GUI).

[CI] Make calling tests easier

Currently, we call each of the tests individually.

cordial/docker/scripts/test_entrypoint.sh

Line 11 in ce6bb61

/bin/bash -c "source /root/catkin_ws/devel/setup.bash && rostest cordial_gui test_cordial_gui_actions.test && rostest cordial_gui test_cordial_gui_pubs_and_subs.test && rostest cordial_manager test_cordial_manager_actions.test && rostest cordial_manager test_cordial_manager_pubs_and_subs.test && rostest cordial_manager test_cordial_manager_services.test"

It would be nice to use test discovery. Perhaps we can use catkin build, which will probably require us to expose tests in the CMakeLists.txt files.

Add Python code coverage tools

For example pytest-cov.

[cordial_manager] Add unit tests to aws_polly_client package

This is an important module and locking it in with tests would allow us to have more courage to refactor it.

Put all parameters for Cordial in one YAML file

This approach will make it easier to understand what can be varied in Cordial.

Run CI periodically

There are two major reasons to run our CI often, even if it's not associated with a push:

1. It will help us find if a dependency breaks our current build
2. It will help us identify flaky tests

Here is one possible way of doing this.

Move docs from cordial-docker

It will be better to have the cordial specific things here.

CI doesn't work for external contributors

See here in #65.

Make README more informative

The readme should mention the following:

• What types of robot behavior this project coordinates
• Mention the role of AWS in the interaction
• Show a picture / gif of the face
• Show a picture / gif of the GUI
• Show what are the public topics, services, and actions
• Explain that it currently uses Kinetic and Python 2.7
• Have an On Contributing section

[cordial_sound] Add tests

It would be nice to add tests here.

Update to Python 3

Cordial uses Python 2.7, which is no longer supported. ROS1 works with Python 3 (here is a tutorial). It would be great to allow Cordial to support Python 3.x.

Dockerfile should build from source each time

The Dockerfile currently builds from an image that has had cordial setup in it. I am not sure that tests the current branch, or the even the latest code in cordial. I think that we should build the full container each time that a push is created (i.e., the Dockerfile should start at the ROS kinetic image, install cordial's dependencies, and build cordial). It will take longer, but there are several reasons to do this:

• It can test the current branch
• It will let us know if any dependencies have been updated and break Cordial

I suspect that the reason an image with cordial setup is used is because it allows us to move the Dockerfile from the top directory. We can probably get around the issue by copying the projects contents beneath the Dockerfile (maybe using a symbolic link?) in the Github action. If this turns out to be difficult and the Dockerfile needs to be in the top directory, so be it.

One thing to realize is that when you make a Docker image, it is like a freeze of that container at a specific time. Your Dockerfile is not run in pulling the image. You just get the previous result of each line in the Dockerfile - that's what the layers are when you're pulling an image. These layers are like binary files, and that's why it's faster to pull an image than rebuild that image from a Dockerfile. It is because of this that code added to the image and packages installed will be in the same version when you pull the image at a later time as when you created the image originally.

Make a workflow to publish a Cordial image to DockerHub

It would be cool to have an automated way of pushing an image of this project to DockerHub. This would make setup with the project faster and easier, as instead of building from a Dockerfile, you could just receive the binary files.

Make failure to have valid AWS credentials more informative

Currently, the following error is raised when there are invalid AWS credentials.

AttributeError: CordialManager instance has no attribute '_aws_client'

It would be nice to make this error more informative by mentioning that the credentials are not working.

This error was mentioned in #18.

Remove unnecessary dependency on NodeJS and NPM

We currently use http-server which is installed with NodeJS and NPM. Python has a built-in http-server, that we can use. This change will require modifying http_server_runner.py.

Add documentation on how to use Cordial

For example:

• What topics do what and what their message type is
• How the http servers work
• An overview of how the manager works

This is probably best done after #5.

[cordial_gui] Add tests

This could be mostly unittests. Integration tests would also be great, but might be harder to do. This will probably be similar to #45.

[cordial_sound] Stream sound, instead of sending it all at once

Currently, sounds to be played is sent in one large message with the full sound to play. It would be much nicer if the sound was streamed to be played in smaller chunks (i.e., a second or less of sound). This would allow dialog to be interupted immediately.

[cordial_face] Refactor the face and remove unnecessary parts

The face is one of the largest pieces of code in this repository. I think a lot of the code there is unnecessary and that the organization could be improved significantly.

Parameterize topic names

Currently, topic names are hard-coded into scripts that use them. It would be better to have all of the topics stored as parameters in ROS's parameter server.

This could be accomplished through having a single YAML file which lists all of the topics for this repository; this YAML file would then be loaded into the parameter server. This approach would also make it easier to control which topics should be public and private, as many topics are used internally and should not be publicly exposed.

Use GitHub Actions for continuous integration

Currently, we have a few tests, but they need to be run manually and are time-consuming. It would be useful to have continuous integration so that we can be sure that changes to the code or package updates won't break anything.

Here's an example repository using Actions for reference.

Create new package to contain all of Cordial's messages

Putting all of Cordial's interfaces into one package would make importing messages much easier.

[cordial_tools] Add tests

It would be great to add tests here.

Add tests for coding style

For example using pytest-flake8

Fix flaky tests

The integration tests sometimes fail when executed in the Docker-based action, usually with the error message "AttributeError: CordialManager instance has no attribute '_aws_client'". Although this error is resolved upon re-running the action, the tests should be fixed so we can be sure that errors are caused by modifications to the code, and not the tests themselves.

Add setup instructions

It would be helpful to have documentation that talks about how to set up and use Cordial.

Make CI trigger on pull requests

Currently CI triggers only on push events (and soon on scheduled intervals #33). I did this because I could not figure out a way to get the branch name for both pull requests and push events.

• For pushes, GITHUB_REF#refs/heads/ returns the branch name after stripping refs/heads from it.
• For pull requests, the best way that I've found is to use github.event.pull_request.head.ref. GITHUB_REF#refs/heads/ returns something with merge and the PR's number, so that doesn't work.

I believe that we'll have have to use conditional logic to avoid having two workflows. Perhaps, this could be an example.

I'm surprised that I'm not able to find a better tool for doing this. I tried this action, which does the same thing as GITHUB_REF#/* (doesn't work for pull requests or pushes with a / in the name, e.g., audrow/feature would be parsed to feature). Perhaps this is worth trying.

[cordial_face] Add tests

This could be mostly unittests. Integration tests would also be great, but might be harder to do.

[cordial_manager] Add integration tests

It's important to have tests so we can ensure things are in a working state and have multiple people contribute.

Features

• Uses rostest
• Runs on Travis CI
  • Use AWS client with credentials

Example tests

• Any messaging (publishers, subscribers, servers, clients, actions)
• Interaction between CordialManager and AwsPollyClient

[CI] Run examples in CI

If possible, it would be great to run the examples in CI, so that we can confirm that they're always working (e.g., we can avoid things like #40).

Scheduled CI keeps failing