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

Bug: partition-view does not work #438

Closed
erikmd opened this issue Oct 6, 2021 · 10 comments · Fixed by #459
Closed

Bug: partition-view does not work #438

erikmd opened this issue Oct 6, 2021 · 10 comments · Fixed by #459
Assignees

Comments

@erikmd
Copy link
Member

erikmd commented Oct 6, 2021

Related issue(s) or PR(s): #421
Related project scope(s) (ex: client, CSS, grading, etc...) : partition-view
Related user(s): @erikmd @yurug

Bug description

I tried using partition-view as described in #421 but I wasn't able to make it work with 0.12 nor with 0.13.0.

To Reproduce

Steps to reproduce the behavior:

  1. Go to Teacher dashboard
  2. Middle-click
  3. Put an existing function name
  4. See error below (screenshot taken with 0.13.0)

Expected behavior

No error

Screenshots

2021-10-07_01-12-46_Screenshot_partition-view_buggy

Current configuration

  • OS: GNU/Linux
  • Browser: Firefox
  • Version: ESR
@AltGr
Copy link
Collaborator

AltGr commented Oct 11, 2021

We probably need to ping @nobrakal here :)

@nobrakal
Copy link
Contributor

It works on a fresh local installation. Do you have any details on your deployment ?

@erikmd
Copy link
Member Author

erikmd commented Oct 12, 2021

OK thanks @nobrakal for looking into this; I'll try to come up ASAP (maybe tomorrow) with a minimal reproducible example with /sync and /repository folders…

@erikmd erikmd added this to the learn-ocaml 0.14.0 milestone Oct 15, 2021
@erikmd
Copy link
Member Author

erikmd commented Oct 15, 2021

Hi @nobrakal, sorry I was swamped with several deadlines; and I'm not sure we can fix this issue for 0.13.1;
so let's postpone this to next week with 0.14.0; I'll send you an e-mail meanwhile

@erikmd
Copy link
Member Author

erikmd commented Nov 3, 2021

Hi @nobrakal, at last I got some time to create a minimal reproducible example, and actually the error appeared immediately after the steps below:

cd learn-ocaml
mkdir sync_repro
…

… cf. its final contents in the attached .zip file: sync_repro.zip

sudo docker run --rm -it -v "$PWD/sync_repro:/sync" -v "$PWD/demo-repository:/repository" \
  -p 8080:8080 ocamlsf/learn-ocaml:0.13.1
# Create two user accounts Alice & Bob.
# Code a correct version of the `times` function for the `demo` exercise.
# Login with the teacher account.
# Open the teacher dashboard.

# Partition the `times` function, which amounts to open:
firefox http://localhost:8080/partition-view.html?id=demo&function=times&prof=30
# → REQUEST ERROR; Could not retrieve data from server; URL not found: Error: Env.Error(_)

# Likewise:
firefox http://localhost:8080/partition/demo/times/30?token=X-U1O-AB1-1FJ-ZOF
# → Error: Env.Error(_)

I took a quick look in the code, and I wasn't able to directly guess the location of the error…

@yurug plans to release 0.14.0 by the end of the week, so if you had the time to have a look, Alexandre, that'd be great!
Otherwise, no worries :)

@nobrakal
Copy link
Contributor

nobrakal commented Nov 5, 2021

Hi there,

Thanks for the detailed example & procedure!

The example you gave me works when running a "local" LearnOCaml (installed with make opaminstall).
I have not tried the Docker installation, but the problem seems to lie there. I will try to investigate :)

erikmd added a commit to pfitaxel/learn-ocaml that referenced this issue Nov 7, 2021
Spotted with (docker, `learn-ocaml build --enable-cmo-build`):

```
LOGS:
Learnocaml v.0.14.0 running.
Updating app at ./www
given_int_0exo           (build .cmo)
Cannot process exercises: Env.Error(_)
```

Note: this latter exception backtrace could be improved (giving more details).

This patch should also fix ocaml-sf#438

However the CI will still fail because of a learn-ocaml-corpus corner case:
```
…
given_persistent_0arrays (build .cmo)
Cannot process exercises: Typemod.Error(_, _, _)
```
@erikmd
Copy link
Member Author

erikmd commented Nov 7, 2021

Hi @nobrakal, thanks for your feedback.

I believe I found the crux of the issue: regarding dependencies, partition-view → asak → compiler-libs,
but it happens that in the final Dockerfile (for minimization purposes), the opam switch is dropped,
while compiler-libs still needs ocamlc and/or the corresponding $(ocamlc -where) directory.
I've just pushed a patch that solves a similar bug in the on-going PR #458, and which will maybe solve #438 as well
(did not test this latter thing)

@erikmd
Copy link
Member Author

erikmd commented Nov 7, 2021

I've just tested the branch from #458, did sudo make docker-images then
sudo docker run --rm -it -v "$PWD/sync_repro:/sync" -v "$PWD/demo-repository:/repository" -p 8080:8080 learn-ocaml, and FYI the bug is indeed fixed.

Unfortunately, the fix increased the size of the server image:

$ sudo docker images
learn-ocaml                           latest                       c9e74cc38cc8   13 hours ago    869MB
ocamlsf/learn-ocaml                   0.13.1                       c3b1d8add5eb   3 weeks ago     103MB

But on the other hand, some standard images (even some "base" ones) already have this order of magnitude:

$ sudo docker images | grep python
python                                3                            7f5b6ccd03e9   17 months ago   934MB

@erikmd
Copy link
Member Author

erikmd commented Nov 7, 2021

OK, so by tweaking my fix and adding just the three following lines in the Dockerfile:

ARG opam_switch="/home/opam/.opam/4.12"

COPY --from=compilation "$opam_switch/bin"/ocaml* "$opam_switch/bin/"
COPY --from=compilation "$opam_switch/lib/ocaml" "$opam_switch/lib/ocaml/"

we end up with:

$ sudo docker images | grep learn-ocaml
learn-ocaml                           latest                       72b68d29feaa   6 minutes ago   675MB

→ I'll merge this fix in master now (so ocamlsf/learn-ocaml:master will be working for people interested in this feature),
and we'll still be able to refine the fix later on if need be (if possible − but I'm unsure we can safely save much more MBs).

erikmd added a commit to pfitaxel/learn-ocaml that referenced this issue Nov 7, 2021
Spotted with (docker, `learn-ocaml build --enable-cmo-build`) from PR ocaml-sf#458:
```
LOGS:
Learnocaml v.0.14.0 running.
Updating app at ./www
given_int_0exo           (build .cmo)
Cannot process exercises: Env.Error(_)
```
Note: this latter exception backtrace could be improved (giving more details).

Close ocaml-sf#438
@nobrakal
Copy link
Contributor

nobrakal commented Nov 8, 2021

Many thanks for the fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants