allenai/ScienceWorld

Python API does not follow PEP 8

AndKaminer opened this issue · 7 comments

Hi there,
I'm newly interested in contributing to this project, and I noticed that the python API doesn't follow python naming convention. I figured (and Dr. Cote suggested) that it would be a good first issue for me to change that API so that it does follow convention. Please let me know if that breaks anything. Otherwise, just let me know, and I'll make a PR.

By the way, is there any specified format/information required for pull requests?

Thanks!

Hi @AndKaminer, thanks for your note. Contributions are very welcome!

The API right now uses camelCase instead of snake_case for function names, and changing this would likely introduce breaking changes (i.e. it would require anyone upgrading ScienceWorld to change their agent code to snake_case). I'm not sure that would yield a great benefit/cost ratio, but of course if others feel it would, then please feel free to submit a PR.

For other contribution ideas, now that some agents (e.g. SwiftSage, CLIN, etc) are starting to have substantially increased performance over the baseline agent, I wonder what it would look like to add in a few more tasks that are harder, or that have more ablations. For example, the change-of-state tasks sometimes have device ablations (e.g. the stove might suddenly be broken in some variations), requiring a modification to the agent's solution strategy (e.g. using a different heat source -- from the blast furnace in the foundry, to building a campfire outside). When we designed the SW tasks, the solutions were originally intended to be a little less formulaic, but the agents of the time (e.g. DRRN, T5 Behavior Cloning Baseline) were so terrible that it seemed like making them more regular would help agents begin to learn them. Now that agents are doing well, I wonder if there are ways of extending the environment's utility (in terms of making tasks harder/less formulaic) that are low-hanging fruit.

Those sounds like great ideas. I will see what I can do about them. I assume more testing is also never a bad thing?

For context, I did suggest @AndKaminer the idea of refactoring the code to be compliant with PEP8. I believe we can do a slow transition and keep the critical methods that use camelCase naming convention has a legacy (but raise a deprecation warning).

Here are some things to be done to be compliant with PEP8 (and more).

  • Use snake case
  • Deprecate old method that uses CamelCase
  • Add Github Action to run tests and flake8 on the code
  • Use flake8 to fix PEP8 errors
  • Fix other scripts (and code examples) that relies on the deprecated methods.

Here are some things to be done to be compliant with PEP8 (and more).

* [X]  Use snake case

* [X]  Deprecate old method that uses CamelCase

* [ ]  Add Github Action to run tests and flake8 on the code

* [ ]  Use flake8 to fix PEP8 errors

* [ ]  Fix other scripts (and code examples) that relies on the deprecated methods.

I'll put up another PR for the remaining.

Oops I didn't mean to close

Here are some things to be done to be compliant with PEP8 (and more).

* [x]  Use snake case

* [x]  Deprecate old method that uses CamelCase

* [x]  Add Github Action to run tests and flake8 on the code

* [x]  Use flake8 to fix PEP8 errors

* [X]  Fix other scripts (and code examples) that relies on the deprecated methods.