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

Added basic REST API to command center #175

Merged
merged 31 commits into from
Jun 2, 2017

Conversation

KamalGalrani
Copy link
Contributor

Basic API. Read REST API section in top level README

@saijel
Copy link
Contributor

saijel commented May 11, 2017

Hey @KamalGalrani, are you done with this PR or are you progressively committing changes? Once you are done committing changes will you let me know so I can review it?

@KamalGalrani
Copy link
Contributor Author

For now I am done with this. I want to add features which will need modification of other modules. Will make another pull request for those before I continue with the API. I'll mail you what I have in mind later.

The Config.py changes you made in #176 may lead to unstable state when a user is logged in from multiple locations. It's better if we change the index from username to something session dependent.

@@ -0,0 +1,26 @@
Microsoft BotFramework interface to LUCIDA
==========================================

Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani can you add a description for the purpose of incorporating this botframework?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

==========================================

## Installation
* Change directory to $LUCIDAROOT/botframework-interface and run `npm install`
Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani, do you think this should go in tools or added to some makefile to make it more automated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* Create a new bot at https://dev.botframework.com/bots/new
- Fill in Name, Bot handle and Description
- Messaging endpoint is the address of the PC running the interface. This has to be https endpoint. For testing purposes one can use ngrok.
* Install ngrok by typing `sudo apt-get install ngrok-client` in terminal
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this to tools or have one script that installs all necessary packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* You may want to add channels on the bot page. Follow the instructions on https://dev.botframework.com/bots.

## Usage
* Change directory to $LUCIDAROOT/botframework-interface and run `node interface.js`
Copy link
Contributor

Choose a reason for hiding this comment

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

Create a makefile and add this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,6 @@
// Copy application ID and password (as explained in README.md) and rename this file to credentials.js
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is okay to go ahead and name this file credentials.js and put in comments where the APP_ID and APP_PASSWORD should go that way we don't have to rely on people change the name of the file. What do you think?

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 prefer it this way because then we can put credentials.js in .gitignore and not worry abt passwords being pushed

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay sure, that makes complete sense.

README.md Outdated

#### Connect client user to Lucida user

This is a one time requiremnt. To connect client user to Lucida user is necessary before the API can process any messages. The flow is as follows
Copy link
Contributor

Choose a reason for hiding this comment

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

Not trying to be nit picky, but there is a type requiremnt should be requirement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

README.md Outdated
```
curl -i -X POST -F "interface=facebook" -F "username=lucida_fb" -F "text_input=Who is this famous celebrity?" -F "file=@/path/to/file/celeb.jpg" http://localhost:3000/api/infer
```

Copy link
Contributor

Choose a reason for hiding this comment

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

I LOVE all of this documentation. I'm wondering if we should include this in a wiki rather than this Readme since this Readme is a beast. We can leave it for now but eventually I think it would be good to make this readme more basic. What do you think?

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 didn't know there was a wiki. I'll put it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if you don't have access to the wiki.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,91 @@
start_time = process.hrtime()
Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani can you put some type of header saying what this file is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was fixed in 20d80ac. You probably didn't pull

var hr_time = process.hrtime()
return time_offset + hr_time[0] + hr_time[1] / 1000000000;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani, can you put all of this time logic (including lines 1-3) in the time function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This too was removed in 20d80ac.

// Setup Restify Server
var server = restify.createServer();
server.listen(3728, function () {
console.log("Listening to bot channels on port 3728...");
Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani, can you put this port in some sort of config file so we keep the programmable parts in one file? We eventually want to get rid of hardcoding ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

addresses[address.channelId] = {channelId: address.channelId, bot: {id: address.bot.id, name: address.bot.name}, serviceUrl: address.serviceUrl, useAuth: address.useAuth};
request.post({
headers: {'content-type' : 'application/x-www-form-urlencoded'},
url: 'http://localhost:3000/api/infer',
Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani, can we add this host to some config file as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if ( result ) {
request.post({
headers: {'content-type' : 'application/x-www-form-urlencoded'},
url: 'http://localhost:3000/api/add_interface',
Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani, same here. Can you add this to a config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@KamalGalrani
Copy link
Contributor Author

I am using phantomjs to automate setting of endpoint. This is particularly required while using ngrok as the endpoint because then you need to set endpoint at almost every run. Can someone please check what I am doing in set_endpoint.js legal. I didn't find any document by Microsoft saying it is illegal just that it is illegal to automate Google search this way so.

Copy link
Contributor

@saijel saijel left a comment

Choose a reason for hiding this comment

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

Once I get this working on my side, let's get this merged in. For new features let's wait and make those new PRs. This way we can get this REST API feature available as soon as possible.


## Start Interface
* Change directory to $LUCIDAROOT/botframework-interface and run `make start_server`
* First run will ask some questions. These can me modified later by editing 'config.sh'.
Copy link
Contributor

Choose a reason for hiding this comment

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

me -> be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,7 @@
all:
npm install
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add a check to make sure npm is installed and if it isn't then install it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if ( retval == 0 ) {
console.log("[INFO] Verifying email address...")
} else {
console.log("[ERROR] Could not connect to Microsoft!!! Will try again in a second...")
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason when I run make start_server for the first time it repeatedly asks me for my BFW_ID and BFW_PW saying [ERROR] Could not connect to Microsoft!!! Will try again in a second... and it is happening at this line. Do you see this on your side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works perfectly here. Follow the instructions I have mailed you and send me the screenshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested in variety of scenarios from personal account, school account, incorrect email, incorrect password. From both India and US based servers. Works perfectly everytime. Couldn't try work account but as far as I know it's the same as school account.

Copy link
Contributor

Choose a reason for hiding this comment

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

screen shot 2017-05-23 at 10 56 12 am
screen shot 2017-05-23 at 10 56 28 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you able to access the url in browser. This seems more of a network issue. Also do send me a screen shot of terminal too. Remove your email id.

Copy link
Contributor

@saijel saijel May 23, 2017

Choose a reason for hiding this comment

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

Yes, I am able to go to the link. I'm wondering if it is my work network. I tried sshing into a linode instance and I'm getting a different error. The Network page is empty.
screen shot 2017-05-23 at 12 56 31 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

screen shot 2017-05-23 at 12 59 21 pm

@KamalGalrani
Copy link
Contributor Author

Added an option to skip automatic update of endpoint.

@KamalGalrani
Copy link
Contributor Author

The issue was with the phantomjs version from Ubuntu 14.04 repository. Fixed now. Just run make all once to update stuff.

Copy link
Contributor

@saijel saijel left a comment

Choose a reason for hiding this comment

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

After going through all of the steps on the README I'm still not able to get it working. I'll send you an email with my steps and let me know if I am doing anything wrong.

start_server:
bash start_interface.sh

.PHONY: all start_server
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a clean function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For clean all you can do is delete node_modules folder. I'll add it anyway

}
EOF

node interface.js $BFW_PORT $CC_HOST
Copy link
Contributor

Choose a reason for hiding this comment

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

node was not found for me. I had to put nodejs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot that in 14.04 node was called nodejs. Will fix this

else
echo ""
echo "Do you want me to save password to file? It will be saved in plain text."
printf "BFW_SAVE_PWD [n]: "
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change BFW_SAVE_PWD [n]: to BFW_SAVE_PWD (y/n):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[blah] is default value. (blah/blah) are options. Added both.

while [ 1 ]; do
echo ""
echo "Enter messaging endpoint (https only) on which Lucida's BotFramework interface is running (e.g. 'https://xxx.ngrok.io'). URLs with exclaimation marks ('!') are not allowed."
echo "If you have no idea what this is you may press enter. Doing so will require you to enter your account password everytime you launch BotFramework interface or save it to config.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused on what I should be doing here. Should I start ngork beforehand? Or by running $make start_server within botframework-interface am I the BotFramework interface for Lucida on a https server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses a running instance if ngrok is running. If not it starts ngrok

@saijel
Copy link
Contributor

saijel commented May 25, 2017 via email

@saijel
Copy link
Contributor

saijel commented May 26, 2017 via email

@KamalGalrani
Copy link
Contributor Author

I'll add a tutorial on how to go around the verifiy login problem

@KamalGalrani
Copy link
Contributor Author

I didn't find any screenshots

@KamalGalrani
Copy link
Contributor Author

Fixed for 14.04.
There are two points mentioned in troubleshooting section of README. Those I won't fix right now. If anyone has an idea about them let me know

@zhexuanc
Copy link
Contributor

zhexuanc commented May 30, 2017

Hi Kamal. I have an issue on this PR.
I start the server remotely using ssh, and I use my local Skype on my laptop to send message to the bot. But there is no reply. Then I keep the server on and try to send message using the Skype on the machine where the server is running. Then there is response. I am not sure that this may caused by network issue.

I try your solution in the troubleshooting section 2 and it works. But there are still messages no response as shown in troubleshooting section 1.

@saijel
Copy link
Contributor

saijel commented May 30, 2017

@KamalGalrani I am having 2 issues on my machine:

    1. When I try automatic updates I am getting a bad gateway error. Here is my terminal and the line in the start_interface.sh (line 277) which is causing the error:
      screen shot 2017-05-30 at 2 35 34 pm
      screen shot 2017-05-30 at 2 35 52 pm
    1. If I set the messaging point manually I am getting the same error that @zhexuanc mentioned. When I send a message from Skype it is not going through to my Lucida instance.

@KamalGalrani
Copy link
Contributor Author

@saijel The problem you are facing is really weird. The only case I see it happen is in the case of network failure midway which is rare. Add these lines to the last step in set_endpoint.js just after the try catch block

console.log(page.url)
console.log(botdata)
console.log(botdata.Message)

@zhexuanc @saijel Did you guys try sending from local Skype after messages from remote Skype were successful. This may be the problem that I have explained in README regarding loss of first few messages after endpoint update.

Also try running from linnode copy under my user. Saijel knows the password. If its running there its some weird setup fault.

@saijel
Copy link
Contributor

saijel commented May 31, 2017

@kamal, since it is working manually on my computer we'll just moved forward. And after doing the Firefox settings we are able to get it working remotely.

I had one more fix that is needed and then we can merge this in.

fi
fi
done < <(netstat -peanut 2>/dev/null | grep LISTEN)
if [ -z $BFW_HOST ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

In my case on my local machine, I get an error start_interface.sh: line 278 [: too many arguments. When asking if[ -z $___ ] we need parenthesis around the variable like if[ -z "$___" ] because the variable could have spaces. Can you change all of the string checks to have ""?

Copy link
Contributor

Choose a reason for hiding this comment

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

@KamalGalrani have you fixed this yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just fixed. Had slipped from my mind.

@saijel saijel merged commit 9e63869 into claritylab:master Jun 2, 2017
@per1234
Copy link

per1234 commented Sep 7, 2017

Now that this has been merged, shouldn't https://github.com/claritylab/lucida/wiki/REST-API be updated?

The REST API is in active development and may change drastically. It is currently available as pull request.

@KamalGalrani
Copy link
Contributor Author

Done

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.

None yet

4 participants