search

BasisResearch / chirho

An experimental language for causal reasoning
https://basisresearch.github.io/chirho/getting_started.html
Apache License 2.0
164 stars 10 forks source link

introduce notebook cleaning and linting scripts with an example #535

Closed rfl-urbaniak closed 3 months ago

rfl-urbaniak commented 3 months ago

This partially resolves #409 .

What is not covered in this PR:

Once we are on the same page about this, I will add the workflow and work on other notebooks one by one, to cover the same notebooks as the ones tested in test_notebooks. This will most likely require manual revisions to notebooks and re-running them completely, so I prefer to do these in separate small PRs.

SamWitty commented 3 months ago

This looks good to me. One thing we might want to consider is just having make format and make lint run both regular code and notebook versions of the shell scripts rather than adding a new make command specific to notebooks.

@eb8680 , any thoughts?

eb8680 commented 3 months ago

To pass lint, all imports are moved to the top of the file, and two function names are modified. Other changes are the result of auto-formatting.

I haven't used nbqa before. Just so I understand, what failure related to the imports wasn't fixed by autoformatting with isort and black? Does this mean running nbqa isort --check <filename> can still fail even if you run nbqa isort <filename> first?

eb8680 commented 3 months ago

One thing we might want to consider is just having make format and make lint run both regular code and notebook versions of the shell scripts rather than adding a new make command specific to notebooks.

I think it's OK to keep the notebook part separate for now.

rfl-urbaniak commented 3 months ago

To pass lint, all imports are moved to the top of the file, and two function names are modified. Other changes are the result of auto-formatting.

I haven't used nbqa before. Just so I understand, what failure related to the imports wasn't fixed by autoformatting with isort and black? Does this mean running nbqa isort --check <filename> can still fail even if you run nbqa isort <filename> first?

I think this is more about flake8 being included in the lint, it sometimes complains about things that make clean doesn't fix, like lines too long or unused imports. I've seen this happen outside of nbqa as well sometimes. In the case of notebooks it also seems that isort sorts imports within cells, but if some imports are not in the top cell, flake8 complains.

Relatedly, I resolved this in the collab repo by adding autoflake --remove-all-unused-imports --in-place ... to the cleaning script. I assume we do not want to remove unused imports in modules as, for example, this is a bit annoying for modules that are WIP, but perhaps could be useful for notebook cleaning, as notebooks will be only added to the list once assumed done?

SamWitty commented 3 months ago

Relatedly, I resolved this in the collab repo by adding autoflake --remove-all-unused-imports --in-place ... to the cleaning script. I assume we do not want to remove unused imports in modules as, for example, this is a bit annoying for modules that are WIP, but perhaps could be useful for notebook cleaning, as notebooks will be only added to the list once assumed done?

Yes, I think it would be a mistake to have make clean automatically remove unused imports. My preference is that we automate things that developers will almost never care about, like import orders or trailing whitespace, but avoid being too heavy handed beyond that.

Anyway, who would you like to review this @rfl-urbaniak ? I always find it confusing when there are two reviewers listed.

rfl-urbaniak commented 3 months ago

Sorry for being confusing. I wanted @eb8680 to approve the general strategy, as this is what would be applied across all notebooks in the repo, and @SamWitty to approve the resulting changes to the notebook,

SamWitty commented 3 months ago

Sorry for being confusing. I wanted @eb8680 to approve the general strategy, as this is what would be applied across all notebooks in the repo, and @SamWitty to approve the resulting changes to the notebook,

Ok, thanks for clarifying. The changes to the notebook have my stamp of approval. @eb8680 , ball's in your court to review the broader PR.