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

To commit or not commit notebook output... #164

Open
mikemhenry opened this issue Oct 9, 2024 · 4 comments
Open

To commit or not commit notebook output... #164

mikemhenry opened this issue Oct 9, 2024 · 4 comments

Comments

@mikemhenry
Copy link
Contributor

mikemhenry commented Oct 9, 2024

I do think that it is best practice to not keep the output BUT we need the output so that our docs render the output:
https://docs.openfree.energy/en/stable/tutorials/rbfe_python_tutorial.html

I think what we should do is maybe have CI do the rendering (it already does the running (at least on these notebooks https://github.com/OpenFreeEnergy/ExampleNotebooks/blob/main/.github/workflows/CI.yml#L68 we will want to make sure those are a 1-1 match of the notebooks we have in our docs))
We could have the result of running the notebooks committed on merge into main? That way it won't clutter up the PR (with each CI run you would need to pull changes) BUT I don't think that would actually help since the output would still be saved...

We could have RTD keep the output which would fix the issue of the docs not having the output BUT that means that if you clone the repo or view the repo with a web browser there still would be no output.

I am not really sure what we should do here, maybe see what OpenFF does? They have the same-ish setup for embedding notebooks in their docs.

This will probably be a bigger issue to work out BUT I think the typo fixes you have here are great 🎉 so could you include the output for now? I will turn this comment into an issue that we can use to discuss what we want to do RE: notebook output being committed.

Originally posted by @mikemhenry in #163 (comment)

@mikemhenry
Copy link
Contributor Author

After more thought, this is what I propose:

We remove the notebook outputs from this repository and never commit them.

When we build the openfe docs, we already pull from this repo, so that is the prefect time to then generate the output. This would ensure our docs have nice looking notebooks (and we can point people to that location in this repositories README) and keep this repo lighter and make the diffs easier to read. This gives us an added check as well that we don't hit any errors when making the outputs using the env file from the openfe repo, which should help us catch any API breaks.

@IAlibay
Copy link
Member

IAlibay commented Oct 10, 2024

@mikemhenry what about the outputs that take a long time to generate? Like when we run an actual transformation?

@mikemhenry
Copy link
Contributor Author

Do we have outputs like that in the notebooks? I had thought we had them be pretty short but said things like "in a production run you would want to run this longer"

@IAlibay
Copy link
Member

IAlibay commented Oct 10, 2024

I think we have a few cases where we have the output from a larger simulation - we'd have to audit.

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

No branches or pull requests

2 participants