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

Improvements to exerciser #625

Closed
mgeisler opened this issue May 9, 2023 · 9 comments
Closed

Improvements to exerciser #625

mgeisler opened this issue May 9, 2023 · 9 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mgeisler
Copy link
Collaborator

mgeisler commented May 9, 2023

The recent #623 reminded me of something I've noticed regarding the exerciser tool, namely that I think we can improve it. Today, I think the workflow has some drawbacks:

  1. It requires people to install another tool, a tool which they mostly need when publishing pages
  2. It requires manual steps to actually become useful from the served pages
  3. It makes the output folder more cluttered than before with the html/ and exerciser/ subdirectories
  4. The new style is inconsistent with the format we already use for Android: notice how the filenames are italic in the Android chapter and monospace in the bare-metal chapter.

I would actually have liked us to a tool which we can run only as part of the publish workflow. I asked for that way back when the exerciser was introduced out of the blue.

Another bad side-effect of the tool is that it requires us to mention lots and lots of files in the course which the students shouldn't actually have to look at. See all the you shouldn’t need to change this comments on https://google.github.io/comprehensive-rust/exercises/bare-metal/rtc.html.

@qwandor, @djmitche, @rbehjati, and others: what are your thoughts?

@rbehjati
Copy link
Collaborator

I agree with you @mgeisler. The process is complicated and not very well described. I had to look at GitHub actions to figure out how to generate the exercises.

The tooling aside, is it even a good idea to have such large exercises with so much template code? I think the exercises should be designed so that the students can understand what is expected of them in no more than 4-5 mins. I have not yet done the elevator exercise from async nor the bare-metal exercise, but based on just a quick look, it feels like there is too much content to go through before starting with the exercise. I think that is also the feedback from @djmitche from teaching the async part.

Can the exercises be simplified, and some of the template code be moved to a crate that the students could just add a dependency to? I am assuming that the students can use most of the template code without having to actually read through the code. #618 might be related, but there is a discussion of the solution vs templates that I did not quite understand :)

@qwandor
Copy link
Collaborator

qwandor commented May 10, 2023

The only people who have to install the tool is people who want to modify exercises and preview them locally. They already have to install the other mdbook tools, so is one more really an issue? We could add a script to install all publishing tools if that would be helpful.

Having to copy the output separately is annoying, I agree. That's automatically done as part of the publish workflow though, and not necessary if you just want to check things locally, as in that case you can use the output directory directly. (In fact that's easier than having to unzip an archive first each time you change something.)

The formatting of filenames we can fix, let's pick what we prefer and change everything to be consistent. My vote is for monospaced.

I'm not sure what you mean by a tool that we only run as part of the publish workflow; isn't that what this is?

Including the contents of the template files all on the exercise page I actually think is an advantage rather than a bad side-effect. When I've taught the course students have usually had questions about how everything fits together, and being able to show them all the different parts in one place without having to switch windows has been convenient. Just because they shouldn't need to change the configuration files and so on doesn't mean they don't want to see them. On the contrary, it mean there's less magic, and it's more clear how everything actually works.

To @rbehjati's comments about the size of the exercises, how would you make the bare metal ones smaller? The compass exercise only has one small Rust file, the rest is configuration for building and flashing, which you can't remove. The RTC exercise, I guess you could move the UART driver and logging code to a separate crate, but in practice students tend to refer to this a lot when writing their RTC driver, so I think it's more useful to keep it in the exercise crate where they can do so easily. The rest is either Rust code they will need to change, build configuration, or assembly to get things to the point that the Rust code can run at all.

@djmitche
Copy link
Collaborator

Regarding the size of the exercises, I agree that the first few days' exercises (probably the first three days?) should be short, throwaway scripts that all fit in one file. However, the last day is a deeper dive and I think it's legitimate to expose students to reading and modifying larger bits of code, as @qwandor suggests.

To the question of putting lots of files in the slide. I agree that it's useful for students to see these files, but I don't think they need to be on the slides. It's easy enough to switch tabs in a presentation and show source code in regular old files from a terminal or IDE.

I also agree that the exerciser tool is complex and undocumented. In a project with a large group of casual contributors, that creates significant friction, so we should do something about it. My understanding is that exerciser handles

  • Rust code in actual .rs files where they can be checked with cargo test etc.
  • Excerpting that code into slides, and
  • Doing so in such a way that only some of the code is visible (for omitting exercise solutions).

We could simplify this by just putting the exercise code -- perhaps only for the specialized topics (day 4) -- into plain old crates and linking to those crates from the slides ("clone this respository and look in the foo/exercises/bar directory"). We could handle the omission of exercise solutions with include! or some variant of it:

// Acquire the forks.
include!("acquire-forks-solution.rs"); // no peeking!

#567 also suggests that we might want to build non-exercise code as part of a crate, and not just inline code. I think mdbook's built-in include functionality could do this. That would also allow the flexibility to include only some of the files relevant to an exercise in the slide (such as the small rust file for the compass exercise).

@rbehjati
Copy link
Collaborator

I see different opinions about including all files in the slides :) I think if it is clear that those files are for just reference, then it is perhaps fine to keep them in the slides. It should be easy to do this for bare-metal RTC, since the slide already says that the students don't have to change those files.

@mgeisler
Copy link
Collaborator Author

The tooling aside, is it even a good idea to have such large exercises with so much template code? I think the exercises should be designed so that the students can understand what is expected of them in no more than 4-5 mins.

I've found this to be a problem in practice: what I hope people can get through in 20-30 minutes ends up taking 45 minutes. The result is that we don't have time to discuss the solutions.

People regularly ask for more exercises in the course feedback. My thinking is to try and change the exercises to make them very bite-sized:

  1. The solution should be trivial to come up with. This makes the Dining Philosophers exercise quite poor since solving it correctly relies on coming up with the "trick" of breaking the symmetry.
  2. The exercises should focus on one part of the language, such as pattern matching or working with strings.

I would like to see ~15 minute exercises every hour. This will spread out the load more evenly and it would force us to make the exercises really focused. I'm sure Nicole will have input here as well after teaching the class a few times 🙂

I'm not sure what you mean by a tool that we only run as part of the publish workflow; isn't that what this is?

I'm suggesting that we don't enable the renderer by default. It can be enabled easily by the publish workflow by setting an environment variable.

We used to do the same for the translation machinery, but we made it a requirement in #461. That allowed @jooyunghan to simplify the setup a bit in #464. I would not be opposed to making them optional again if we have nice wrapper scripts to use in the publish workflow (that functionality could go into mdbook-i18n-helpers/ themselves).

The only people who have to install the tool is people who want to modify exercises and preview them locally.

Not quite, since everything fails when the tool isn't installed:

% mdbook build
2023-05-10 22:34:20 [INFO] (mdbook::book): Book building has started
2023-05-10 22:34:20 [INFO] (mdbook::book): Running the exerciser backend
2023-05-10 22:34:20 [INFO] (mdbook::renderer): Invoking the "exerciser" renderer
2023-05-10 22:34:20 [ERROR] (mdbook::renderer): The command `mdbook-exerciser` wasn't found, is the "exerciser" backend installed? If you want to ignore this error when the "exerciser" backend is not installed, set `optional = true` in the `[output.exerciser]` section of the book.toml configuration file.
2023-05-10 22:34:20 [ERROR] (mdbook::utils): Error: Rendering failed
2023-05-10 22:34:20 [ERROR] (mdbook::utils): 	Caused By: Unable to start the backend
2023-05-10 22:34:20 [ERROR] (mdbook::utils): 	Caused By: No such file or directory (os error 2)

This is different from not installing mdbook-i18n-helpers since we only use the preprocessor:

% mdbook build
2023-05-10 22:45:40 [INFO] (mdbook::book): Book building has started
2023-05-10 22:45:40 [WARN] (mdbook::preprocess::cmd): The command wasn't found, is the "gettext" preprocessor installed?
2023-05-10 22:45:40 [WARN] (mdbook::preprocess::cmd): 	Command: mdbook-gettext
2023-05-10 22:45:40 [INFO] (mdbook::book): Running the html backend

Just because they shouldn't need to change the configuration files and so on doesn't mean they don't want to see them. On the contrary, it mean there's less magic, and it's more clear how everything actually works.

Right, there shouldn't be any magic: they should look at the files via a proper editor. That's also what I used to show people the directory structure and to explain the relationship between the files. I believe that gives a much better impression of the structure than an overly long page.

I know I introduced some longer pages for the exercises in the Rust Fundamentals part... I regret if that somehow made it appear that listing 5+ long files (with copyright headers and all!) is a good idea. The downloadable zip files is a great idea and we should build on it to make the exercise pages look like they belong in the same course as the rest of the pages.

@mgeisler
Copy link
Collaborator Author

I know I introduced some longer pages for the exercises in the Rust Fundamentals part... I regret if that somehow made it appear that listing 5+ long files (with copyright headers and all!) is a good idea.

I'm fixing the copyright headers in #1212.

@mgeisler mgeisler added good first issue Good for newcomers help wanted Extra attention is needed labels Sep 25, 2023
mgeisler pushed a commit that referenced this issue Sep 26, 2023
**Task** Addresses:
#625
Certain pages in the mdbook that display filenames have inconsistent
styling.

The style we want to follow: [the interop
section](https://google.github.io/comprehensive-rust/android/interoperability/java.html)

The current problem: [the RTC
driver](https://google.github.io/comprehensive-rust/exercises/bare-metal/rtc.html)
excercise page.

--
@mgeisler I checked this feature by looking through every page


![work](https://github.com/google/comprehensive-rust/assets/38759997/8affd0c2-71f2-4708-88f6-f63cf3c24efa)
@djmitche
Copy link
Collaborator

I think I misunderstood exerciser -- the templating bit is built into mdbook, while exerciser produces a directory/zip file containing the files from the exercises, based on ` directives.

I think that's still useful, and I think it's been fairly stable since this was filed. Maybe this issue can be closed or the specific tasks broken out into child issues?

@proski
Copy link
Contributor

proski commented Jan 31, 2024

This makes the Dining Philosophers exercise quite poor since solving it correctly relies on coming up with the "trick" of breaking the symmetry.

I share your sentiment. Please see #1772 (async version for now).

@djmitche
Copy link
Collaborator

The "foundations" course has now been refactored to have more, shorter exercises. That doesn't include the dining-philosophers problem, and I suspect the problem may go away entirely when the revisions are complete ( in #1536).

This issue seems to have morphed into a general discussion of exercises, and that's a bit overly-broad to handle here, so I'm going to close it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants