Prototype of an HTMLInterface

ebolyen commented 11 years ago

~~NOT READY FOR MERGING~~ Hey everyone,

Just putting this here for scrutiny. There is still a lot of refactoring I need to do.

The HTMLInterface as an interface and its handlers aren't completely fleshed out.

Currently in order to download the file, the download_list_of_strings handler needs to return a dictionary with a "return" key. This is stupidly fragile and I need to fix it. The _output_handlers method of the interface class should be able to handle this, but there needs to be some introspection on the handlers themselves.

Currently I have only stubbed out an HTMLInterface for make_command of pyqi.

To start the HTMLInterface server type this: "pyqi serve-html-interface -m pyqi.interfaces.HTMLInterface.config -p 8080"

ghost commented 11 years ago

Can one of the admins verify this pull request?

gregcaporaso commented 11 years ago

ok to test

ghost commented 11 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/37/

ghost commented 11 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/38/

ebolyen commented 11 years ago

I am working on improving the way you specify an output should be downloaded. Also I think there should be a way to describe the rendering of a page. There are a couple options here:

Single Class

outputs = [

#Here the InputName describes what the filename should be, and DownloadResult is a flag
    HTMLInterfaceResult(Parameter=cmd_out_lookup('result'),
                   Handler=download_list_of_strings,
                   InputName='download-file',
                   DownloadResult=True),
#Here would be a success page which has a flag for Page, there could only be one of these
    HTMLInterfaceResult(Parameter=cmd_out_lookup('page'),
                   Handler=display_list_of_strings,
                   Display=True,
                   MIMEType="text/html")

]

or

outputs = [
#Same as above, but the InputName has been inlined into the DownloadResult
    HTMLInterfaceResult(Parameter=cmd_out_lookup('result'),
                   Handler=download_list_of_strings,
                   DownloadResult='download-file'),

    HTMLInterfaceResult(Parameter=cmd_out_lookup('page'),
                   Handler=display_list_of_strings,
                   Page=True,
                   MIMEType="text/html")

]

Multiple Subclasses:

outputs = [

   Download(Parameter=cmd_out_lookup('result'),
                   Handler=download_list_of_strings,
                   FileNameLookup='download-file'), #There could also be a FileName='file.txt' property

    Page(Parameter=cmd_out_lookup('display'),
                   Handler=display_list_of_strings,
                   MIMEType="text/html")

]

I feel that the multiple subclasses make for a cleaner API for the interface, but I want to know what you all think.

gregcaporaso commented 11 years ago

I don't see any reason why having multiple subclasses of your HTMLInterfaceResult wouldn't work, and I think it makes a lot of sense. @wasade, I haven't used these objects yet so maybe you could weigh in?

gregcaporaso commented 11 years ago

@ebolyen, thanks for getting this started!

As we discussed, the next steps should be a code clean-up, and nicer output formatting. You mentioned the code is really messy right now and there are a lot of things that you want to clean up, so I'm not doing a very detailed code review, but see my initial comments above.

I'd be great if you could have these changes in by next Friday (11/15). Does that sound like a reasonable goal? I will email you to set up a time. After our next meeting, @jrrideout and @wasade should try out the code. @wasade, note that there is one question for you in the discussion above.

jairideout commented 11 years ago

Thanks @ebolyen! I'm looking forward to playing around with this and reviewing the code. Let me know when you guys are ready for me to do so.

ebolyen commented 11 years ago

@gregcaporaso That should work fine. I'm just happy to have something working and definitely want to clean up the config interface and core code.

ebolyen commented 10 years ago

Would there be value in creating an "index" HTMLInterface-config that could override a default HTMLInterface index?

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/39/

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/40/

gregcaporaso commented 10 years ago

@jrrideout, do you know why the jenkins result hasn't updated on this PR? It says Waiting to hear about 20d196c, but it looks like the test passed if we look at the build results page.

jairideout commented 10 years ago

Don't know- if the jenkins page says it passed, should be okay

I'll have the tests rerun just to be sure

jairideout commented 10 years ago

retest this please

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/42/

jairideout commented 10 years ago

OK, looks good

wasade commented 10 years ago

Should I review this?

On Thu, Nov 21, 2013 at 9:02 AM, Jai Ram Rideout notifications@github.comwrote:

OK, looks good

— Reply to this email directly or view it on GitHubhttps://github.com/bipy/pyqi/pull/203#issuecomment-28996538 .

gregcaporaso commented 10 years ago

Not yet, waiting for @ebolyen to post that it is ready for review (should be by 12:00 MT on Friday 11/22).

On Thu, Nov 21, 2013 at 11:00 AM, Daniel McDonald notifications@github.comwrote:

Should I review this?

On Thu, Nov 21, 2013 at 9:02 AM, Jai Ram Rideout notifications@github.comwrote:

OK, looks good

— Reply to this email directly or view it on GitHub< https://github.com/bipy/pyqi/pull/203#issuecomment-28996538> .

— Reply to this email directly or view it on GitHubhttps://github.com/bipy/pyqi/pull/203#issuecomment-29007343 .

wasade commented 10 years ago

Sounds good, thanks

On Thursday, November 21, 2013, Greg Caporaso wrote:

Not yet, waiting for @ebolyen to post that it is ready for review (should be by 12:00 MT on Friday 11/22).

On Thu, Nov 21, 2013 at 11:00 AM, Daniel McDonald <notifications@github.com <javascript:_e({}, 'cvml', 'notifications@github.com');>>wrote:

Should I review this?

On Thu, Nov 21, 2013 at 9:02 AM, Jai Ram Rideout <notifications@github.com <javascript:_e({}, 'cvml', 'notifications@github.com');>>wrote:

OK, looks good

— Reply to this email directly or view it on GitHub< https://github.com/bipy/pyqi/pull/203#issuecomment-28996538> .

— Reply to this email directly or view it on GitHub< https://github.com/bipy/pyqi/pull/203#issuecomment-29007343> .

— Reply to this email directly or view it on GitHubhttps://github.com/bipy/pyqi/pull/203#issuecomment-29007666 .

ebolyen commented 10 years ago

Ready for review:

Run setup for pyqi first, or navigate to the pyqi directory before running the following command:

"pyqi serve-html-interface -m python_interface_config_module_here -p port"

Example:

"pyqi serve-html-interface -m pyqi.interfaces.HTMLInterface.config"

Then open a browser and navigate to http://localhost:8080

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/43/

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/44/

wasade commented 10 years ago

Just pulled and ran it. This is awesome, very excited. Will go over the code soon. Regarding multiple subclasses on outputs, agree that it is cleaner

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/46/

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/47/

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/48/

jairideout commented 10 years ago

Just tried out the interface- AWESOME!!! You have no idea how excited QIIME users will be to have this functionality in place. :)

Here are some high-level comments before I start the code review. I realize this is still under development and very alpha, so apologies if these were already on your to-do list (definitely not trying to be nitpicky or critical of your work). Some of these items may be okay to address later once the pull request is merged.

if a required file isn't provided (e.g., download-file), when the page reloads, all of the previously filled in values are lost. This will likely frustrate users
the landing page (e.g., what I get when I navigate to http://localhost:8080) just has a bulleted list of commands. It'd be nice to clean this page up and add some content and styling
would be nice to list default values for options if no input is specified by the user
would be helpful if the pyqi serve-html-interface command prints out the URL that users should navigate to, as well as telling them how to kill the server (similar to the ipython notebook command)
before moving onto the BIOM project, can you add configs for the other pyqi commands?
I like the ability to download results, as well as possibly view them in the browser. It'd be neat if the download-file input box was removed, and hitting Submit would always take you to a new page. This new page would then indicate whether the command ran successfully or not, and have a link to download the result(s) or view them within the browser (if the output is something that can/should be displayed). Right now, to toggle between downloading or viewing a result inline, you have to edit the config file. Could the outputs instead have flags or something that indicate whether they are download-only, viewable inline-only, or both, and then this would tell the interface how it should render results? What do you guys think about this idea?

Your configurable templating/styling idea is a good one, but I'd rather get this merged first and then add those features later on, since they aren't as critical. @gregcaporaso @wasade do you agree?

gregcaporaso commented 10 years ago

Your configurable templating/styling idea is a good one, but I'd rather get this merged first and then add those features later on, since they aren't as critical. @gregcaporaso @wasade do you agree?

Yes, I agree on both points (it's a good idea, but let's focus on getting the core functionality merged first).

jairideout commented 10 years ago

Another general comment (this is a minor one):

You created two new directories, pyqi/interfaces/HTMLInterface and pyqi/core/interfaces/HTMLInterface. The corresponding optparse directories are named optparse. I think the name HTMLInterface is a bit redundant here because it is already under an interfaces/ directory. I'd also like to keep consistency in the capitalization scheme we're using. Does it make sense to rename these directories to html?

ebolyen commented 10 years ago

if a required file isn't provided (e.g., download-file), when the page reloads, all of the previously filled in values are lost. This will likely frustrate users

Yeah, I hadn't gotten around to that yet. That is a pretty common UX optimization.

the landing page (e.g., what I get when I navigate to http://localhost:8080) just has a bulleted list of commands. It'd be nice to clean this page up and add some content and styling

This can be done pretty easily, but an overridable templating system would be the best solution

would be nice to list default values for options if no input is specified by the user

Good idea

would be helpful if the pyqi serve-html-interface command prints out the URL that users should navigate to, as well as telling them how to kill the server (similar to the ipython notebook command)

Good idea, hadn't thought of that!

before moving onto the BIOM project, can you add configs for the other pyqi commands?

Absolutely

I like the ability to download results, as well as possibly view them in the browser. It'd be neat if the download-file input box was removed, and hitting Submit would always take you to a new page. This new page would then indicate whether the command ran successfully or not, and have a link to download the result(s) or view them within the browser (if the output is something that can/should be displayed). Right now, to toggle between downloading or viewing a result inline, you have to edit the config file. Could the outputs instead have flags or something that indicate whether they are download-only, viewable inline-only, or both, and then this would tell the interface how it should render results? What do you guys think about this idea?

I like the initial result page, however the only way to make this possible is if the server was capable of remembering it has a page to show you. This requires the execution to happen outside of the server and for the server to tag each execution with an ID for later recall.

As far as being able to choose as a user unless prevented by the config: No output which is provided for a file will render correctly in a browser. We could just replace all \n with <br /> however I don't know if the browser will copy the results correctly.

Also, this would prevent subclassing the output results which provide a slightly richer API for dealing with download vs page in a clean way.

You created two new directories, pyqi/interfaces/HTMLInterface and pyqi/core/interfaces/HTMLInterface. The corresponding optparse directories are named optparse. I think the name HTMLInterface is a bit redundant here because it is already under an interfaces/ directory. I'd also like to keep consistency in the capitalization scheme we're using. Does it make sense to rename these directories to html?

I had been wondering about that for a while, I like html as a name, but everyone called it an HTMLInterface, so I just went with that. Renaming is fine by me.

jairideout commented 10 years ago

Thanks @ebolyen!

Here's my thoughts on what needs to happen now (i.e., in order to have the pull request merged) and what can be postponed:

keeping track of filled-in values: if this is relatively easy to do, can you add this functionality to the pull request?
landing page listing commands: definitely agree with the configurable templating system being the better long-term option, but I think for now it'd be good to have some basic info and formatting/styling put in place, similar to what you have for the individual command pages. This should be relatively easy to put in place (hard-coded for now) and I think this is important to have so that we can demo the interface to others. I'm just thinking of something as simple as some sort of welcome message, and a sentence or two that points users to the list of available commands. Does this sound reasonable for this pull request?
default values: since this should be trivial, can you add this functionality to the pull request?
pyqi server printing URL and instructions: should be trivial, so would be good to add at this point
filling in other configs: not sure if this needs to happen in this pull request or not. Would be nice to have more commands so that we can have a more impressive demo. How about adding at least one other config for a command?
results page, download vs view results: Since these changes aren't trivial, let's postpone for now. I think once we start using the interface as it is, with more commands and integration in BIOM, we may start to get a better sense of what changes (if any) need to happen.
renaming HTMLInterface directories to html: this is important to put in place before merging, as imports to this functionality will be affected if we change the name later. Should also be easy to do.

@gregcaporaso @wasade what do you guys think? I know we want this functionality added ASAP but I think it'd be good to clean up a few of the rough edges now.

Will continue with more line-by-line reviewing.

jairideout commented 10 years ago

@ebolyen I'm done with my line-by-line review.

wasade commented 10 years ago

@jrrideout I agree regarding the recommended list of things to do and postponement

ebolyen commented 10 years ago

Made the corrections I could. Still need to work on the todo list

ebolyen commented 10 years ago

Apparently renaming files nuked our prior conversation.

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/49/

jairideout commented 10 years ago

Yeah, that's one of the really annoying things that GitHub does. You can still see the old conversation if you scroll up and click each of the "Show outdated diff" links (which is annoying).

jairideout commented 10 years ago

@ebolyen can you please let us know when you're ready for us take another pass through?

ebolyen commented 10 years ago

Willl do, should be sometime this afternoon. On Nov 25, 2013 7:49 AM, "Jai Ram Rideout" notifications@github.com wrote:

@ebolyen https://github.com/ebolyen can you please let us know when you're ready for us take another pass through?

— Reply to this email directly or view it on GitHubhttps://github.com/bipy/pyqi/pull/203#issuecomment-29206908 .

jairideout commented 10 years ago

Great, thanks!

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/51/

ebolyen commented 10 years ago

Ready for review. @wasade @jrrideout

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/52/

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/53/

jairideout commented 10 years ago

Thanks @ebolyen! There are a few remaining issues, most of which should be easy to fix I think:

make-optparse: I'm getting the following error. I'm passing pyqi.commands.make_optparse.MakeOptparse as input to the command option:

'str' object has no attribute 'CommandIns'

make-command: the test-code option is always set to True, even when I select the False radio button.
make-bash-completion: the output file has each character printed on its own line.

Please let me know when these issues are fixed and then I'll retest/re-review the code.

ebolyen commented 10 years ago

~~@jrrideout What browser are you using in regards to the true/false radio buttons not working? I cannot replicate that issue.~~ Misunderstood the error. Correcting it now.

ebolyen commented 10 years ago

@jrrideout @wasade Ready for review again.

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/54/

ghost commented 10 years ago

Build results will soon be (or already are) available at: http://ci.qiime.org/job/pyqi-github-pr/55/

biocore / pyqi

Prototype of an HTMLInterface #203

Single Class

Multiple Subclasses: