Applying linting to notebooks

[CompRhys]

Updates to use a more recent version of ruff from astral rather than the initial action written for early version before astral was formed around ruff. Applies ruff to notebooks inside the repo.

Added nbstripout action to remove unnecessary information from the tutorial notebooks to reduce the diffs that are expected when run by different users

[jduerholt]

Hi @CompRhys,

thanks for your PR. I am unsure about the nbstripout, as I find it nice to have the outputs in the notebooks for didactic purposes, especially as we link then sometimes also directly into mkdocs. @bertiqwerty: what do you think?

It seems that you are much more experienced with Ruff than me, what do you think of using Ruff also for formatting and get rid of black? https://docs.astral.sh/ruff/formatter/

Would you mind to implement this?

Best,

Johannes

[bertiqwerty]

Hi @CompRhys,

thanks for your PR. I am unsure about the nbstripout, as I find it nice to have the outputs in the notebooks for didactic purposes, especially as we link then sometimes also directly into mkdocs. @bertiqwerty: what do you think?

It seems that you are much more experienced with Ruff than me, what do you think of using Ruff also for formatting and get rid of black? https://docs.astral.sh/ruff/formatter/

Would you mind to implement this?

Best,

Johannes

Hi all,

I am in favor of using a new Ruff-version and also of using ruff format instead of Black.

About the notebooks, I agree with Johannes that it is good to show the output.

[CompRhys]

Okay! sure can update the linting actions to make use of the pre-commit settings such that there's only one place where the ruff version is controlled. I have removed all the obvious # noqa: <xyz> and # type: ignore comments and then will go back and try fix/re-add where necessary for pyright. Switched to using the pyright ci action suggested by microsoft https://github.com/microsoft/pyright/blob/main/docs/ci-integration.md. When running the notebooks they're spammed with warnings about EI vs LogEI, I will rerun to generate the outputs before this gets merged but first do you guys agree that it makes sense to swap all the defaults/tutorials to use LogEI? #444 and https://github.com/Radical-AI/bofire/tree/logei-defaults

[CompRhys]

@jduerholt @bertiqwerty I have done the above updates and rebased so that we kept the outputs that were there before (I have kept the nbstripout action as is cleans up some user specific metadata but configured it to keep the outputs). Testing on my local branch there are now some tests that are failing, I am looking at these now and will let you know when I have fixed them such that they can be re run here and maybe this can be merged

[CompRhys]

ideally we could move this inside the pre-commit action but there appear to be issues with getting pyright to work with pre-commit

[CompRhys]

I think the tests that were failing are flaky, here they passed but the same code being tested on my fork failed.

[jduerholt]

I think the tests that were failing are flaky, here they passed but the same code being tested on my fork failed.

Yeah, there are some flaky tests, espececially in the DoE module, but @Osburg is currently working on a a refactoring of the whole module which will hopefully also cure the flaky tests ;)

Thanks for the PR. @bertiqwerty, I think it makes sense that you also have a look on it as most of the pipelines and checks were implemented by you.

[jduerholt]

One think that catched my attention:

What do you think of adding a pyright section to pyproject.toml (https://github.com/microsoft/pyright/blob/main/docs/configuration.md). There we could then for example define that it is not throwing an error when one overrites variables or methods, which would significantly reduce the number of type: ignore in the code (https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportIncompatibleMethodOverride).

[bertiqwerty]

One think that catched my attention:

What do you think of adding a pyright section to pyproject.toml (https://github.com/microsoft/pyright/blob/main/docs/configuration.md). There we could then for example define that it is not throwing an error when one overrites variables or methods, which would significantly reduce the number of type: ignore in the code (https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportIncompatibleMethodOverride).

I think we should keep the type ignores because incompatible override is not very nice and at least we should mark it as such.

[jduerholt]

I think we should keep the type ignores because incompatible override is not very nice and at least we should mark it as such.

Hmm, but we do it a lot in the data models (to guarantee proper serialization and deserialization using the type attribute), just have a look at the new type: ignore added in this PR ...

[bertiqwerty]

This just means that we have a lot of type violations because a Literal["something"] is not a str (if it were a str you could assign any value of type str which you cannot). Maybe we should replace type: str by type: Any in the base class because the derived types are never str but always some kind of Literal["something"]. I have not found a way to restrict this to any kind of Literal, hence just Any.

[jduerholt]

Hmm, ok. Then we keep it as it is currently and perhaps fix this in a later PR.

[CompRhys]

So when looking at pyright I removed them all and then added all the ones that created issues back and then added a bunch of extra ones where it was now complaining. I wasn't really sure as to why the changes I made created new typinging issues as I only manually changed one bit of logic to cast an dataframe to numpy before calling ravel due to an incoming pandas deprecation.

If you guys are happy with this PR as it is I think it would be good to merge and then this pyright stuff and then also changing the tutorials to follow the LogEI best practice can be done in another PR(s)

[jduerholt]

I think, that there is a new pyright version, which is flagging more than the old one, that we have used so far ....

[CompRhys]

I pinned the version in the action to the version that was in the setup.py file previously so for the action I don't think so but perhaps as I was fixing locally I probably am using the most recent version due to pylance which might be the cause

[jduerholt]

Thanks for this effort @CompRhys . Looks good to me! After @bertiqwerty had a look we can merge it ;) Best, Johannes

[bertiqwerty]

Thanks a lot @CompRhys. Looks good to me!