researchart / rose7re20

3 stars 1 forks source link

Reviews on submission 125 #8

Open fabianodalp opened 4 years ago

fabianodalp commented 4 years ago

The assigned reviewers are going to post their reviews on this submission within this issue. The same thread will be used also to support the interaction with the authors.

Reviewers, please check STATUS.md to determine which badges the artifact is applying for. A description of the badges can be found here: https://re20.org/index.php/artifacts/. You will also receive an e-mail with further instructions shortly.

colinwerner commented 4 years ago

Able to install the required libs; however, not able to run on MacOS yet!

Something to do with getopt. Needs more investigation ... or help from @andivogelsang ?

fwiesweg commented 4 years ago

Dear Colin,

I originally developed it on Linux, so there is probably a series of corner cases for MacOS that we have not covered. I suppose you're at the step of executing ./run.sh? A bit of research indicates that getopt as provided by MacOS does not support long options: https://stackoverflow.com/questions/12152077/how-can-i-make-bash-deal-with-long-param-using-getopt-command-in-mac

There's a branch ("macos") ready in our repository replacing GNU getopts by the bash-builtin getopt that should make it work. If there are any further issues, don't hesitate to ask. https://github.com/NaPiRE/project_riskdrivenRE/tree/macos

The repository is currently private, but should become public within a few hours so you can pull it easily.

Best regards, Florian

michaelrath-work commented 4 years ago

Hi, I installed your artifact inside a Ubuntu 20.04 docker container. Installation works. However, when executing the run.sh script, the web UI does not work. The console output reads

Starting napire analysis REST service on http://127.0.0.1:8888
┌ Error: (ErrorException("type Regex has no field match_data"), Base.StackTraces.StackFrame[getproperty(::Regex, ::Symbol) at Base.jl:33, exec(::Regex, ::String, ::Int64) at parseutils.jl:4, exec(::Regex, ::String) at parseutils.jl:4, parse_request_line!(::String, ::HTTP.Messages.Request) at Parsers.jl:159, parse_start_line!(::String, ::HTTP.Messages.Request) at Messages.jl:481, readheaders(::HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}, ::HTTP.Messages.Request) at Messages.jl:474, startread(::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at Streams.jl:153, handle_transaction(::HTTP.Handlers.var"#4#5"{HTTP.Handlers.RequestHandlerFunction{napire.web.var"#68#74"{Dict{NamedTuple,NamedTuple}}}}, ::HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}; final_transaction::Bool) at Servers.jl:332, (::HTTP.Servers.var"#handle_transaction##kw")(::NamedTuple{(:final_transaction,),Tuple{Bool}}, ::typeof(HTTP.Servers.handle_transaction), ::Function, ::HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}) at Servers.jl:328, handle_connection(::Function, ::HTTP.ConnectionPool.Connection{Sockets.TCPSocket}, ::Int64, ::Int64) at Servers.jl:291, macro expansion at Servers.jl:249 [inlined], (::HTTP.Servers.var"#8#9"{HTTP.Handlers.var"#4#5"{HTTP.Handlers.RequestHandlerFunction{napire.web.var"#68#74"{Dict{NamedTuple,NamedTuple}}}},Base.RefValue{Int64},Int64,Int64,Bool,HTTP.ConnectionPool.Connection{Sockets.TCPSocket},Sockets.TCPSocket,Int64})() at task.jl:358])
└ @ HTTP.Servers ~/.julia/packages/HTTP/GN0Te/src/Servers.jl:255

I used the code from here https://github.com/researchart/rose7re20/tree/master/submissions/125-vogelsang.

Regards,

Michael

fwiesweg commented 4 years ago

Dear Michael,

seems as if a piece of API has been removed in Julia 1.3. Unfortunately, that error happens quite deep in a library we're using. I just tried to upgrade the whole stack but that's pretty invasive.

You can make it work reliably on Julia 1.1.1, though, which can be downloaded here: https://julialang.org/downloads/oldreleases/

It just needs to be extracted and added to PATH before starting run.sh.

wget https://julialang-s3.julialang.org/bin/linux/x64/1.1/julia-1.1.1-linux-x86_64.tar.gz
tar -xf julia-1.1.1-linux-x86_64.tar.gz
export PATH=/home/fwiesweg/Zwischenspeicher/julia-1.1.1/bin/:$PATH
julia -v   # should print "julia version 1.1.1"
napire/run.sh

Best regards, Florian

michaelrath-work commented 4 years ago

Hi Florian,

switching to the older julia version solves the problem. I can see the evaluation interface on /web. However, /userweb is blank. Is that the correct behavior?

Best regards, Michael

fabianodalp commented 4 years ago

Thank you all for the good discussion so far. Just a heads up: not too relevant for this track, but it is important to know that using older versions of these libraries raises quite some security concerns, see what GitHub automatically flagged about the submission :)

image

colinwerner commented 4 years ago

I got it working on MacOS (with one small change https://github.com/NaPiRE/project_riskdrivenRE/pull/1) and downgrading Julia to 1.1.1.

fwiesweg commented 4 years ago

Dear Michael,

no, you should definitely see something alike to the blue-ish screenshot in the paper. Do you have JavaScript turned on or are you using some non-standard browser? That's the most common reason when this happens to me.

Otherwise, I suppose it's some transient issue about the compilation of the angular application, since it worked nicely for me this morning. These are difficult to debug without some experience in the JavaScript environment, so I've got two simple options: 1) you could try to delete userweb/node_modules and userweb/build-prod and start run.sh again, which will recompile the whole user-facing web application. It should work afterwards. 2) you could try the following branch to which I added the pre-compiled application, which I tested in Firefox and Chromium a few seconds ago. You can try starting run.sh -n, which skips the compilation, or ./run.sh without arguments if the former doesnt work. https://github.com/NaPiRE/project_riskdrivenRE/tree/angular

Best wishes, Florian

fwiesweg commented 4 years ago

@colinwerner I merged your changes, thank you for the patch!

Concerning fabiano's comment on security issues, these appear mostly like XSS issues somewhere down the angular stack. Given that the application is currently served via pure HTTP without encryption or authentication, it'd be way easier to just intercept the traffic itself than to inject some JavaScript into the website and get the data from there.

Unfortunately, such minor details are hard to catch in an automated way so Github does not tell us loudly ;-)

For the moment, the application should not be exposed to the public internet in the first place and only be used locally or via a VPN (which applies to basically every piece software developed for research). We're far from a fully-fledged productive deployment of the application in a security-sensitve context, so these can safely be ignored.

michaelrath-work commented 4 years ago

Hi Florian,

thanks for your reply. I think it has to do with my specific setup. Because of all the versioning stuff (getop, julia, etc), I always use a docker setup and everything else is not acceptable for me. However, according to the guidelines here [1], this is not required for your artifact and it won't affect my judgement.

Here is what I did so far. First, I created a docker file with the following content

FROM ubuntu:20.04

ENV TZ=Europe/Berlin
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone
ARG DEBIAN_FRONTEND=noninteractive

RUN apt-get update \
    && apt-get install -y curl wget npm graphviz w3m

WORKDIR /root

RUN wget https://julialang-s3.julialang.org/bin/linux/x64/1.1/julia-1.1.1-linux-x86_64.tar.gz \
    tar xf julia-1.1.1-linux-x86_64.tar.gz

and build it with docker build --tag re2020-126 .

Next, I cloned your repository, cd'ed into it and executed

docker run -it  \
    --name "re2020-126" \
    --volume "`pwd`:/data" \
    -p 18888:8888 \
    re2020-126 \
    /bin/bash

Inside the docker container, I finished the julia setup

cd /root
export PATH=/root/julia-1.1.1/bin:$PATH

and then I ran your code

cd /data
bash ./run.sh

Once, everything has started (which takes really, really long ... stupid node), it should be available from my host machine using http://127.0.0.1:18888/web and http://127.0.0.1:18888/userweb, according to my defined port forwarding. But that doesn't work. However, inside the container, I'm able to see the page using w3m http://127.0.0.1:8888/web ... it is ugly (not the page, just how w3m renders it in the console), but it works . But w3m http://127.0.0.1:8888/userweb does not. I think this may be a limitation of w3m, which cannot handle complex javascript, if at all.

I still cannot figure out why the port forwarding to my host machine does not work. According to this answer [2], it may have something to do with your explicit binding to 127.0.0.1. Therefore I searched and modified your code (I'm not familiar with julia, btw, but I tried my best). Specifically I changed in napireweb.jl the lines 557 and 587 to

        HTTP.serve(r -> respond(apispec, r), "0.0.0.0", port)

Now, I'm able to run inside the container w3m http://0.0.0.0:8888/web. So I think I made correct changes. But still, w3m http://0.0.0.0:8888/userweb brings up nothing, not even an error message. And my ultimate goal, using your app on my host machine with a real browser on http://127.0.0.1:18888/web and http://127.0.0.1:18888/userweb is not working at all.

According to @colinwerner, who used a traditional setup, your artifact is working. He uses MacOS, the same as I do. Therefore I suggest, your artifact is working as expected, and I could also manage to get it running halfway. However, IMHO, I would strongly suggest to create a docker or image based setup, to avoid all the platform/versioning issues and vague terms like unix-ish operating system and recent-ish version of npm / julia. Maybe my example code is a good starting point: still not perfect, because it includes manual steps (julia install, ...) and does not mention explicit package versions.

Best Wishes, Michael

[1] https://re20.org/index.php/artifacts/ [2]https://stackoverflow.com/a/57427805

fwiesweg commented 4 years ago

Dear Michael,

yes using w3mwill most likely not work. I haven't tried it, but angular uses all sort of fancy modern web technologies for which there might not be enough polyfills. Concerning the /web endpoint, yes, I know its ugly... I thought I could get away with some jQuery and minor JS hacking, but there turned out to be way more features necessary than I expected.

For a fixed reproduction package, you're right and it does probably make sense to hand in something like a container or VM image. It'd be really nice to have some indication in the requirements for artifacts which container runtimes and formats would be preferred for the review process so we can make sure they're available and run smoothly.

Best regards, Florian

colinwerner commented 4 years ago

Aside from the artifact review, which will continue anon, but perhaps a few dumb questions: 1) I cannot find the paper (I am looking to see this famed screenshot to ensure what I am getting is what you are expecting I get :)

This is what I see: image

2) I think this is GREAT! It seems promising, but I'd like a few examples. Do 'a b c' and you see '1 2 3' which means you are awesome! Or something. I certainly can tinker around with it, but I'm lost.

PS: I didn't read the paper (I cannot find it).

andivogelsang commented 4 years ago

Hi Colin, all,

I added the accepted paper (not yet CRV) to the repo. I'd advise you to use the userweb endpoint, which is reachable via http://127.0.0.1:18888/userweb It is easier to understand and matches the one shown in the paper. If you enter the evidences given in our case study (Table VI), you should get a result that looks like the data in Table VII.

Best regards, Andreas

colinwerner commented 4 years ago

@andivogelsang thanks!

(Seems like I wasn't exactly following directions ;-P)

image

michaelrath-work commented 4 years ago

Hi @colinwerner, @andivogelsang,

according to the aimed status, which is available and reusable, we (the reviewers) are not required or supposed to read the accompanied paper. IMHO, and according to [1], this is only necessary for reproducible and results replicated. So we 'just' have to check that we can access and use the artifact, independent of the actual results.

Michael

[1] https://re20.org/index.php/artifacts/

colinwerner commented 4 years ago

@michaelrath-work understood. I just wanted to make sure what I was viewing was the intended artifact.

fabianodalp commented 4 years ago

Hi @colinwerner @michaelrath-work the paper is indeed not necessary, but great that @andivogelsang has shared -- I believe it is even better when we have full alignment between artifact and paper!

colinwerner commented 4 years ago

The authors are applying for the badges of Reusable and Available.

Available The artifacts are available from a DOI through Figshare (which is documented in STATUS.md).

Reusable I was able to utilize the application with a little bit of effort. I was able to select various effects and problems and within a few minutes the tool told me the probable underlying causes. It was a neat demonstration and I commend the authors for making it available (and reusable).

As Michael pointed out, it would be more useful (ie reusable) if it was easier to run on all platforms with the latest libraries.

I would suggest to update the installation instructions to reflect the limitations of the package (e.g. julia v1.1.1). Also, I would remove the installation instructions from the readme to avoid duplication).

Finally, it would be neat if this tool was hosted somewhere, perhaps NaPiRE's web site?

I recommend both Available and Reusable badges.

michaelrath-work commented 4 years ago

Evaluation

The submission aims for Available and Resuable.

Available

The artifact can be access via link on figshare.

Notes: none

Reusable

Notes: The vague terms recent-ish linux/julia etc need to be more specific (e.g. it only works with julia version 1.1.1). Therefore, please update these requirements.

IMHO: At best, a docker setup with appropriate Dockerfile would be perfect and simplify usage.

Result

I recommend both Available and Reusable badges.

fabianodalp commented 4 years ago

Thank you all for the good discussion and for the changes. We will award both badges here: available and reusable. @andivogelsang @fwiesweg please fix the last round of minor comments and I think we are good to go. Also, make sure your camera-ready paper points to the DOI that makes the badge available.

fwiesweg commented 4 years ago

I just updated the artifact on figshare, reflecting the suggestions. In addition, I included a pre-compiled version of the userweb interface, since compiling Angular applications without experience in the Javascript ecosystem can sometimes become a day's work.

As for the Dockerfile, I'll keep that in mind for the next time. It's definitely a very good idea for research artifacts!

Concerning hosting, the calculations can easily become very resource intensive (details are in the paper), and the backend has not been tested extensively for security issues. So while possible in the future, it requires some more thought to do so in a safe and affordable way.

Thank you all very much for your feedback!