espressomd / espresso

The ESPResSo package
https://espressomd.org
GNU General Public License v3.0
226 stars 183 forks source link

Simplify code review of tutorials #3930

Closed jngrad closed 3 years ago

jngrad commented 3 years ago

Currently, solution cells contain python code wrapped in fenced blocks in markdown cells. The idea was to prevent the user from running the solutions by clicking on Run All. However, during hands-on sessions, tutors make sure the participants go through the cells one by one. During self-teaching, participants would presumably also go through cells one by one, because clicking Run All would put them at risk of triggering one of the the many assertion statements written between solutions to check participants' code.

Having the solution code wrapped in markdown cells makes it more difficult for tutorial authors and reviewers to run the solutions and edit them. It also makes the CI logic for tutorials more complex and fragile - we missed a bug in #3872, which had to be fixed in a tutorial PR (d6b4d97d8). It's also unclear - without inspecting the python code - what should happen when a solution markdown cell contains a fenced python code block with regular text around it. For future reference, here is the relevant python logic:

https://github.com/espressomd/espresso/blob/46fd5f133a78c9ed7731743e425c035b7d0b751e/doc/tutorials/html_runner.py#L150-L157

It would be simpler from a maintenance perspective to leave code solutions as code cells. This requires no change to CI.

If we approve this simplification, the snippet of code above becomes unused and can be removed in the distant future.

RudolfWeeber commented 3 years ago

I find it important that solutions are hidden and that students who do the tutorials seriously attempt the exercises without seeing the solution.

Learning by doeing is much more effective than learning by reading.

Tutors will have to run the tutorials manually before teaching them, anyway, to be prepared themselves. So I’m not too worried about serious issues not getting noticed in time.

jngrad commented 3 years ago

I find it important that solutions are hidden and that students who do the tutorials seriously attempt the exercises without seeing the solution.

This is probably a misunderstanding. This issue is not about hiding solutions, which we agree is important for didactic reasons. This is about making our life easier as maintainers and reviewers. Right now, to review and improve a tutorial, I have to convert all markdown cells to code cells, remove the backticks, do the changes and run the cells, add backticks, convert to markdown cells. Repeat this process every time you change something.

So I’m not too worried about serious issues not getting noticed in time.

The bug I mentioned was caught by chance. It only occurs once in a week, when we deploy tutorials to the ESPResSo website.

RudolfWeeber commented 3 years ago

This is probably a misunderstanding. This issue is not about hiding solutions, which we agree is important for didactic reasons. This is about making our life easier >as maintainers and reviewers.

OK. Then I did not understand it. Let’s take it to the next core team jour fixe.

jngrad commented 3 years ago

It's no longer an issue from my side, thanks to tooling:

# convert solution cells to hidden code cells
/tikhome/jgrad/Documents/protocols/DevOps/exercise2/converter.py --to_py -i 04-lattice_boltzmann_part2.ipynb
# convert solution cells to hidden markdown cells
/tikhome/jgrad/Documents/protocols/DevOps/exercise2/converter.py --to_md -i 04-lattice_boltzmann_part2.ipynb
RudolfWeeber commented 3 years ago

Offlien discussion: JN's scripts to do the cell type conversion will be integrated

KaiSzuttor commented 3 years ago

what's the conclusion here?