automl / CARL

Benchmarking RL generalization in an interpretable way.
https://automl.github.io/CARL/
Apache License 2.0
128 stars 10 forks source link

Readd rna main #78

Closed amsks closed 1 year ago

amsks commented 1 year ago

I branched this from the main branch and added changes from readd_rna.

codecov[bot] commented 1 year ago

Codecov Report

Merging #78 (dcf25ef) into main (8ce8c0a) will decrease coverage by 1.21%. The diff coverage is 0.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #78 +/- ## ========================================== - Coverage 24.67% 23.46% -1.21% ========================================== Files 65 70 +5 Lines 4065 4274 +209 Branches 553 579 +26 ========================================== Hits 1003 1003 - Misses 3010 3219 +209 Partials 52 52 ``` [![Impacted file tree graph](https://codecov.io/gh/automl/CARL/pull/78/graphs/tree.svg?width=650&height=150&src=pr&token=Lb8jKfW2S8&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=automl)](https://codecov.io/gh/automl/CARL/pull/78?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=automl)
TheEimer commented 1 year ago

Agreed, docstring updates would be good, though it seems like those answers are in the comments already! Additionally (and Caro pointed this out, I just wanted to highlight it), it kind of seems like the RNA wrapper env is doing quite a bit of heavy lifting in terms of e.g. action space definition. That's not necessarily bad, but as long as it's convenient we should probably have a relatively uniform structure with a clear base env and a clear carl wrapper.

TheEimer commented 1 year ago

We should definitely fix the pre-commit check before merging

TheEimer commented 1 year ago

The precommit checks are failing due to gitlab username stuff - we shouldn't merge this into main but fix here. Why is this done via gitlab anyway? Does this also fail on main? @benjamc @amsks

TheEimer commented 1 year ago

The precommit checks are failing due to gitlab username stuff - we shouldn't merge this into main but fix here. Why is this done via gitlab anyway? Does this also fail on main? @benjamc @amsks

Also: why are we using flake8 here when our linter is mypy? We should follow up on this, replace flake8 with mypy and probably not use gitlab if we can avoid it. If we can't we should create a token for that. First though: who set up the original pre-commit workflow? Was that you, @benjamc ? Is this from the automl repo template?

eddiebergman commented 1 year ago

@TheEimer flake8 and mypy serve different but overlapping purposes.

Flake8 is a general linter which does some type checking but focuses on code quality. Mypy focues on type checking with some limited code quality. Both are usually compatible with each other :)

I highly recommend ruff as a replacement it even provides some automated fixes to some problems, but that's beside the point.

eddiebergman commented 1 year ago

With respect to the gitlab thing, flake8 used to be hosted there but the author changed to github. There's no tokens or anything that need to be used, those links in the .pre-commit.yaml are just links to the source code to download the library.

eddiebergman commented 1 year ago

Lastly, you can always run pre-commit auto-update locally... to well, auto-update the .pre-commit.yaml file :)

TheEimer commented 1 year ago

Do we care about the patch codecov? A lot of that is the original learna code, right? Are there any other todos left?