EnergySystemsModellingLab / MUSE_OS

Welcome to the MUSE-OS repository
https://muse-os.readthedocs.io/en/latest/
GNU General Public License v3.0
22 stars 8 forks source link

Remove files autogenerated by Sphinx #287

Closed alexdewar closed 1 month ago

alexdewar commented 1 month ago

We don't need simulation results etc. stored in the repo. They are used for the documentation, but are generated as needed anyway. They don't appear to be used for anything else and will likely just get out of sync when the code changes.

Let's remove them and lighten the repo a bit.

dalonsoa commented 1 month ago

But it would seem they are not autogenerated, at least not all of them.

alexdewar commented 1 month ago

Gah... I thought I'd got rid of the errors.

alexdewar commented 1 month ago

Ok, so I think I've fixed everything now!

I've added a step to the CI to run muse --model default before checking the notebooks, which will create the requisite Results folder. I think this makes sense, because the tutorial does say to run this command at the start.

There is currently a hack in there: I'm symlinking the Results folder into docs/ because, for some reason, running-muse-example.ipynb assumes the files will be in there instead. Let me know if you'd like a less smelly solution.

I've left the Results folders under docs/tutorial-code as they actually are used by notebooks all over the place. Potentially we could remove these too at some point, but I think that's a job for another day.

alexdewar commented 1 month ago

Ah, that reminds me of something I forgot to say!

So the weird thing is that the documentation builds fine, regardless of whether the results are there or not.

The reason the CI pipeline fails is because in documentation.yml it tests the Jupyter Notebooks before building the documentation, so if the notebooks fail to run, then the documentation won't get built. I might be wrong here, but it seems like two tasks are being conflated currently: check that the notebooks run and check that the documentation builds (and possibly publishing it). Really, I think the notebooks should be being checked separately, maybe as an additional step in ci.yml where all the other invocations of pytest live.

What do you think?

dalonsoa commented 1 month ago

Nope. This was done this way on purpose to ensure that if the notebooks didn't run, the documentation workflow failed to succeed, as a way of flagging the documentation potentially going out of sync. Otherwise, the documentation builds but, as you will see if you open it, there will be a lot of missing bits related to data not found. Or that is what should happen, at least.

alexdewar commented 1 month ago

Ohh, I see. That makes sense.

Either way though, the user doesn't need to run anything before generating the docs with Sphinx, so I don't think we need to update the instructions.

alexdewar commented 1 month ago

What do you think, @dalonsoa? Can we merge?

dalonsoa commented 1 month ago

OK, let's merge, but I don't understand how the documentation built locally by developers will be correct - even if the built does not fail - if Results are needed to plot stuff.