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

WIP: continuous monitoring support #67

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

breznak
Copy link
Member

@breznak breznak commented Jan 6, 2016

Fixes: #17

};

// main "load" function that supports both URL/local file
$scope.loadFile = function(event) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this can be removed, but I thought if you could change the buttons to work with only this method? So there'd be only 1 button "Load". Do you think it'd work?

@breznak
Copy link
Member Author

breznak commented Jan 6, 2016

@jefffohl for your preliminary review, if you have time. thx

@breznak
Copy link
Member Author

breznak commented Jan 7, 2016

@jefffohl can you please review this?
The monitoring/online updates are working as a draft.
I have a problem with the large (2M+) file, re-reading it in PapaParse take some time, but I know the updatade file size, if possible, I'd like to seek/skip to the bytes read and only read the new bytes. I'm somewhat doing it with conditional skipping in the chunk call, but it's still slow

}
$scope.canDownload(); // will set the local file= true/false
loadFileHelper();
setMonitoringTimer(appConfig.POLLING_INTERVAL); //FIXME create an entry element for numeric value in UI for this, each change should call setMonitoringTimer()
Copy link
Member Author

Choose a reason for hiding this comment

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

@jefffohl could you do the UI for this? Some way to enter numeric values, 0 means disabled, >0 means delay in polling interval for monitoring in ms. On a change the element should call the setMonitoringTimer()

Conflicts:
	build/app.js
	client/src/app/appConfig.js
	client/src/app/appCtrl.js
@breznak
Copy link
Member Author

breznak commented Jan 13, 2016

resolved merge conflicts, hope I haven't introduced any bugs during the merge.

@breznak
Copy link
Member Author

breznak commented Jan 19, 2016

@jefffohl how much of this PR are you reusing in your work on #78 ? I was thinking about rebasing a PR off this PR to have only the working functionality, introduction of the Angular's $interval and possibly limited support for monitoring with diff files only. I hope the work on proxy should then be separate and further extend the monitor. capabilities.

@jefffohl
Copy link
Member

I am not using any of this code, as it seemed that the approach wouldn't work. I am sorry if I wasn't clear about that. I don't think we should be working on the same feature at the same time.

I am currently working on the server implementation, which is required for online monitoring.

@breznak
Copy link
Member Author

breznak commented Jan 19, 2016

Thanks Jeff.
So it would be ok if I go ahead with

introduction of the Angular's $interval and possibly limited support for monitoring with diff files only

from this PR? It would not intervene with any of your work on the server?

@jefffohl
Copy link
Member

To be honest, I don't think it is a good idea to keep working on this PR. The code is too divergent between our two branches, and it is going to be tough to merge them. It might be best to wait until I am finished with my server work, and then, if you want to implement a method for monitoring files using a diff method and the fileReader API, you can do that after I have finished the server. At that point, I am hoping you will be able to rebase the server work on to this branch. I don't think it is wise for us to be working concurrently on these features, as there is so much code overlap.

@breznak
Copy link
Member Author

breznak commented Jan 19, 2016 via email

@jefffohl
Copy link
Member

I am hoping to have it ready before Friday, but there are some things I haven't figured out how to do yet. If I get stuck, I will let you know, and ask for help.

@jefffohl
Copy link
Member

@breznak - Sorry, I am still working on this. Making progress though. I hope that this is not holding you back from things that you need to do.

@jefffohl
Copy link
Member

@breznak - sorry still working on this. Some things came up this week that I had to prioritize.

@jefffohl
Copy link
Member

jefffohl commented Feb 4, 2016

@breznak - I am working on this issue on this branch: https://github.com/jefffohl/nupic.visualizations/tree/issue78

There are a number of fundamental changes happening here. Specifically:

  • Parsing the CSV files on the server using the node module, csv-parse
  • Using socket.io to communicate with the server. This allows the client and server to communicate with each other using events. This will be useful in handling files that are continuously updating.
  • Removed the "Browse..." button, as using this does not allow us to create a file handle that can be updated, so it is no longer needed.

Remotely hosted files can be parsed, as well as local files. For local files, you must give a full path from the system root to the file. Note that because of this feature, the code as it currently stands should NEVER be hosted on a public server, as that will give the public access to all the files on the web server. We will need to figure out a good way to make sure that this feature is disabled if the app is ever hosted publicly. Right now, the intent is that the app should only be run locally.

Local files can be continuously updated. I added a little bash script: /examples/tests/update.sh that will continuously update the no_timestamp.csv file. You can use this to test the continuous update feature.

If you want to test this branch as it is, first make sure that node and npm are installed. Then run npm install followed by npm start

@jefffohl
Copy link
Member

@breznak I have a working prototype on this branch: https://github.com/jefffohl/nupic.visualizations/tree/issue78

This version allows you to "play" a local file. You can enter the path to a local file, and it will load a portion of the file. Clicking the "play" button will start to stream the file from the file system into the web app.

Next steps are:

  • Allow the user to adjust the size of the window - that is, the range of data to graph
  • Allow the user to adjust the speed of play.
  • Allow the user to "scrub" across the file backwards and forwards to find the spot to graph.

@breznak
Copy link
Member Author

breznak commented Feb 25, 2016

@jefffohl sorry for the delay, I was off the grid

That sounds great, will you make a PR?

@jefffohl
Copy link
Member

I could. I was thinking that I would get some of the other steps completed first, but I can make a PR if you like. I am on vacation right now, but I can do it when I return on Monday.

@jefffohl
Copy link
Member

@brezank - I am thinking of making a new PR for what I have now in https://github.com/jefffohl/nupic.visualizations/tree/issue78

Note that this is almost a complete rewrite of the architecture, due to parsing the data now on the server. Some additional things I also changed along the way:

  • Modified the GUI layout
  • Changed the way notifications are presented
  • Modified the threshold highlighting function. I found that having the threshold highlights having padding was creating problems. If there were many highlights close together, they would all blend together, giving the user a misleading sense of what is happening. I changed the highlighting function to very precisely mark where the data is exceeding the threshold. I realize that I should have made this a separate PR. If this bothers you, let's discuss.

Let me know your thoughts.

@breznak
Copy link
Member Author

breznak commented Mar 22, 2016

@jefffohl thanks Jeff, sorry for the silence, I was somehow missing all notifications from this project :/
Will test over the weekend.. IMHO sounds good.

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.

2 participants