Added an OCaml autograder #18
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThe pull request introduces several new files to set up and grade an OCaml lab exercise. A new Dockerfile creates an OCaml environment using the Changes
Sequence Diagram(s)Docker Image Build FlowsequenceDiagram
participant Docker
participant Dockerfile
participant System
participant GitHub
Docker->>Dockerfile: Start build process
Dockerfile->>System: Install OCaml environment and packages (base, yojson)
Dockerfile->>System: Create users (autolab, autograde) and directories
Dockerfile->>GitHub: Clone Tango repository
Dockerfile->>System: Navigate to Tango/autodriver and run "make"
System-->>Dockerfile: Autodriver built
Dockerfile->>System: Copy autodriver binary to /usr/bin/ and set setuid permissions
Dockerfile->>System: Remove Git and clean up cloned files
Dockerfile-->>Docker: Final image ready
Autograder Build and Test FlowsequenceDiagram
participant User
participant Makefile
participant CopyProcedure
participant DuneBuild
participant DuneTest
User->>Makefile: Execute "make all"
Makefile->>CopyProcedure: Run "copy" target (extract autograde.tar and copy toposort.ml)
CopyProcedure-->>Makefile: Files prepared
Makefile->>DuneBuild: Enter build directory and run "dune build"
DuneBuild-->>Makefile: Build completed
Makefile->>DuneTest: Run "dune test"
DuneTest-->>Makefile: Test results returned
Makefile-->>User: Display build and test output
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
OCaml-toposortlab/autograde-Makefile (1)
1-11: Makefile could be more robust with error handling and additional safeguardsThe Makefile works correctly for the basic flow, but could benefit from some improvements to make it more robust:
- It assumes
autograde.tarexists but doesn't verify this- No error handling if the tar extraction or copy operations fail
- No verification if
toposort.mlexists before copyingConsider adding error handling and checks to make the Makefile more robust:
all: copy copy: + @if [ ! -f autograde.tar ]; then echo "Error: autograde.tar not found"; exit 1; fi + @if [ ! -f toposort.ml ]; then echo "Error: toposort.ml not found"; exit 1; fi tar -xvf autograde.tar cp toposort.ml toposortlab_dune/lib + @if [ ! -d toposortlab_dune ]; then echo "Error: extraction failed"; exit 1; fi cd toposortlab_dune && \ dune build && \ dune test clean: + @if [ -d toposortlab_dune ]; then \ rm -r toposortlab_dune; \ + fiOCaml-toposortlab/README.md (1)
1-30: Fix Markdown formatting issues for better readabilityThe content is informative, but there are a couple of markdown formatting issues:
- Heading levels should only increment by one level at a time (h1 → h2 → h3)
- The fenced code block is missing a language specifier
- Missing a clear document structure with sections for setup, usage, etc.
Here's a suggested fix:
# OCaml-toposortlab -### Assessment Language +## Assessment Language OCaml -### Autograder Language +## Autograder Language OCaml -### Autograding Environment Packages +## Autograding Environment Packages The following opam packages: - yojson - base A Dockerfile is provided to install the necessary packages. -### Assessment Scenario +## Assessment Scenario Students are required to implement a topological sort function in OCaml matching a graph interface. -### Handin Format +## Handin Format Single file called toposort.ml -### autograder.tar Directory Content +## autograder.tar Directory Content The standard directories in a Dune project: -``` +```text bin/ lib/ test/ dune-project<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 3-3: Heading levels should only increment by one level at a time Expected: h2; Actual: h3 (MD001, heading-increment) --- 24-24: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>OCaml-toposortlab/Dockerfile (1)</summary><blockquote> `1-30`: **Dockerfile can be optimized for better security and build efficiency** The Dockerfile works but has some areas for improvement: 1. Uses absolute paths inconsistently (flagged by Hadolint) 2. Multiple RUN commands can be consolidated to reduce image layers 3. No explicit USER switching after setting up users 4. No ENTRYPOINT or CMD specified Here's a suggested optimization: ```diff FROM ocaml/opam:ubuntu-20.04-ocaml-4.10 -RUN opam install base yojson -RUN eval $(opam env) +RUN opam install base yojson && eval $(opam env) USER root # Install autodriver -WORKDIR /home -RUN useradd autolab -RUN useradd autograde -RUN mkdir autolab autograde output -RUN chown autolab:autolab autolab -RUN chown autolab:autolab output -RUN chown autograde:autograde autograde -RUN git clone https://github.com/autolab/Tango.git -WORKDIR Tango/autodriver -RUN make clean && make -RUN cp autodriver /usr/bin/autodriver -RUN chmod +s /usr/bin/autodriver +WORKDIR /home +RUN useradd autolab && \ + useradd autograde && \ + mkdir autolab autograde output && \ + chown autolab:autolab autolab && \ + chown autolab:autolab output && \ + chown autograde:autograde autograde && \ + git clone https://github.com/autolab/Tango.git && \ + cd /home/Tango/autodriver && \ + make clean && make && \ + cp autodriver /usr/bin/autodriver && \ + chmod +s /usr/bin/autodriver # Clean up -WORKDIR /home -RUN apt-get remove -y git -RUN apt-get -y autoremove -RUN rm -rf Tango/ +WORKDIR /home +RUN apt-get remove -y git && \ + apt-get -y autoremove && \ + rm -rf Tango/ # Check installation -RUN ls -l /home -RUN which autodriver +RUN ls -l /home && which autodriver +# Switch to non-root user for security +USER autograde +WORKDIR /home/autograde + +# Set default command when container starts +CMD ["/bin/bash"]🧰 Tools
🪛 Hadolint (2.12.0)
[error] 17-17: Use absolute WORKDIR
(DL3000)
OCaml-toposortlab/toposort.ml (1)
10-21: Improve adjacency list handling for clarity and efficiencyThe current adjacency list handling works but could be improved:
- The naming doesn't clearly indicate that it's mapping from a node to its outgoing edges
- The pattern matching to handle missing keys is less concise than using value_map
let visited = Hash_set.create (module Int) in (* Construct the adjacency list representation of the graph*) - let adj_list = Int.Map.of_alist_multi g.edges in + (* Map from node to its outgoing neighbors *) + let outgoing_edges = Int.Map.of_alist_multi g.edges in let rec dfs_helper (acc : int list) (node : int) = if Hash_set.mem visited node then acc else let () = Hash_set.add visited node in - let neighbors = (match Int.Map.find adj_list node with - | Some neighbors -> neighbors - | None -> []) in + let neighbors = Int.Map.find outgoing_edges node |> Option.value ~default:[] in let acc = List.fold ~init:acc ~f:(dfs_helper) neighbors in node :: acc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
OCaml-toposortlab/autograde.taris excluded by!**/*.tar,!**/*.tar
📒 Files selected for processing (4)
OCaml-toposortlab/Dockerfile(1 hunks)OCaml-toposortlab/README.md(1 hunks)OCaml-toposortlab/autograde-Makefile(1 hunks)OCaml-toposortlab/toposort.ml(1 hunks)
🧰 Additional context used
🪛 Hadolint (2.12.0)
OCaml-toposortlab/Dockerfile
[error] 17-17: Use absolute WORKDIR
(DL3000)
🪛 markdownlint-cli2 (0.17.2)
OCaml-toposortlab/README.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
24-24: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
Comment on lines
+8
to
+23
| let topological_sort (g : graph) : int list = | ||
| let visited = Hash_set.create (module Int) in | ||
| (* Construct the adjacency list representation of the graph*) | ||
| let adj_list = Int.Map.of_alist_multi g.edges in | ||
| let rec dfs_helper (acc : int list) (node : int) = | ||
| if Hash_set.mem visited node then | ||
| acc | ||
| else | ||
| let () = Hash_set.add visited node in | ||
| let neighbors = (match Int.Map.find adj_list node with | ||
| | Some neighbors -> neighbors | ||
| | None -> []) in | ||
| let acc = List.fold ~init:acc ~f:(dfs_helper) neighbors in | ||
| node :: acc | ||
| in | ||
| List.fold ~init:[] ~f:(dfs_helper) g.nodes |
There was a problem hiding this comment.
Topological sort algorithm has an issue with cycle detection
The topological sort implementation works well for DAGs (Directed Acyclic Graphs), but doesn't handle cycles correctly. In the presence of cycles, the algorithm will still produce output but it won't be a valid topological sorting.
Consider adding cycle detection:
open Core
type graph = {
nodes : int list;
edges : (int * int) list;
}
let topological_sort (g : graph) : int list =
let visited = Hash_set.create (module Int) in
+ let temp_visited = Hash_set.create (module Int) in
(* Construct the adjacency list representation of the graph*)
let adj_list = Int.Map.of_alist_multi g.edges in
+ let rec dfs_helper (acc : int list) (node : int) =
+ if Hash_set.mem temp_visited node then
+ failwith "Cycle detected in graph"
+ else if Hash_set.mem visited node then
acc
else
- let () = Hash_set.add visited node in
+ let () = Hash_set.add temp_visited node in
let neighbors = (match Int.Map.find adj_list node with
| Some neighbors -> neighbors
| None -> []) in
let acc = List.fold ~init:acc ~f:(dfs_helper) neighbors in
+ let () = Hash_set.add visited node in
+ let () = Hash_set.remove temp_visited node in
node :: acc
in
- List.fold ~init:[] ~f:(dfs_helper) g.nodes
+ try
+ List.fold ~init:[] ~f:(dfs_helper) g.nodes
+ with
+ | Failure msg -> failwith ("Topological sort failed: " ^ msg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let topological_sort (g : graph) : int list = | |
| let visited = Hash_set.create (module Int) in | |
| (* Construct the adjacency list representation of the graph*) | |
| let adj_list = Int.Map.of_alist_multi g.edges in | |
| let rec dfs_helper (acc : int list) (node : int) = | |
| if Hash_set.mem visited node then | |
| acc | |
| else | |
| let () = Hash_set.add visited node in | |
| let neighbors = (match Int.Map.find adj_list node with | |
| | Some neighbors -> neighbors | |
| | None -> []) in | |
| let acc = List.fold ~init:acc ~f:(dfs_helper) neighbors in | |
| node :: acc | |
| in | |
| List.fold ~init:[] ~f:(dfs_helper) g.nodes | |
| open Core | |
| type graph = { | |
| nodes : int list; | |
| edges : (int * int) list; | |
| } | |
| let topological_sort (g : graph) : int list = | |
| let visited = Hash_set.create (module Int) in | |
| let temp_visited = Hash_set.create (module Int) in | |
| (* Construct the adjacency list representation of the graph*) | |
| let adj_list = Int.Map.of_alist_multi g.edges in | |
| let rec dfs_helper (acc : int list) (node : int) = | |
| if Hash_set.mem temp_visited node then | |
| failwith "Cycle detected in graph" | |
| else if Hash_set.mem visited node then | |
| acc | |
| else | |
| let () = Hash_set.add temp_visited node in | |
| let neighbors = (match Int.Map.find adj_list node with | |
| | Some neighbors -> neighbors | |
| | None -> []) in | |
| let acc = List.fold ~init:acc ~f:(dfs_helper) neighbors in | |
| let () = Hash_set.add visited node in | |
| let () = Hash_set.remove temp_visited node in | |
| node :: acc | |
| in | |
| try | |
| List.fold ~init:[] ~f:(dfs_helper) g.nodes | |
| with | |
| | Failure msg -> failwith ("Topological sort failed: " ^ msg) |
dwang3851
reviewed
Feb 27, 2025
There was a problem hiding this comment.
Overall, the changes look good, but when I tried to run the assessment on autolab locally, it didn't work, and it returns error "/bin/sh: 2: dune: not found." When I ran the commands in the docker image locally, I also ran into the same error. Did you run it on your local instance of autolab? I've never used Ocaml before so I'm unsure if I messed up my docker image install or if there is an issue with the docker image.
Also, a small nit is you should include the uncompressed autograde.tar in the Ocaml-toposortlab directory.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.