Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Feature Request] DevOps Features #545

Closed
1 task done
0xprofessooor opened this issue Aug 17, 2021 · 4 comments
Closed
1 task done

[Feature Request] DevOps Features #545

0xprofessooor opened this issue Aug 17, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@0xprofessooor
Copy link
Contributor

🚀 Feature

I think it would be cool to add a variety of DevOps features:

  1. Pre commit hooks (e.g. black, isort). For example see below:
repos:
  - repo: https://github.com/ambv/black
    rev: 21.7b0
    hooks:
    - id: black
      language_version: python3.8

  - repo: https://github.com/timothycrosley/isort
    rev: 5.9.2
    hooks:
    - id: isort
      args: [--filter-files, --profile=black]
      files: \.py$
  1. Introduce poetry https://python-poetry.org/ it's a really good dependency and packaging manager that replaces the setup.py script with its own automatic virtual environment handling. See an example toml spec below:
[tool.poetry]
name = "anvil"
version = "0.1.0"
description = "A library for rapid prototyping and testing of new ideas in RL."
authors = ["Rohan Tangri <rohan.tangri@gmail.com>"]

[tool.poetry.dependencies]
python = ">=3.7,<3.10"
pydantic = "^1.8.2"
torch = "^1.9.0"
numba = "^0.53.1"
numpy = "^1.21.2"

[tool.poetry.dev-dependencies]
pytest = "^6.2.4"
flake8 = "^3.9.2"
black = "^21.7b0"
isort = "^5.9.2"
pre-commit = "^2.13.0"

[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

  1. Add a script, something like setup_dev.sh, which can be run to setup development environment. See below for an example:
#!/bin/sh
echo ---------------------
echo Installing and creating poetry virtual environment
echo ---------------------
pip3 install poetry
poetry install
echo ""

echo ---------------------
echo Installing pre-commit hooks
echo ---------------------
pip3 install pre-commit
pre-commit install
echo ""

echo ---------------------
echo DONE
echo ---------------------

Motivation

It would be nice to have a one-stop easy setup of the environment, and poetry has a lot of community support! (for reference, it's what we use at JP Morgan now).

Pitch

A clear and concise description of what you want to happen.

Alternatives

A clear and concise description of any alternative solutions or features you've considered, if any.

Additional context

Add any other context or screenshots about the feature request here.

### Checklist

  • I have checked that there is no similar issue in the repo (required)
@0xprofessooor 0xprofessooor added the enhancement New feature or request label Aug 17, 2021
@araffin araffin added the Maintainers on vacation Maintainers are on vacation so they can recharge their batteries, we will be back soon ;) label Aug 17, 2021
@araffin
Copy link
Member

araffin commented Aug 23, 2021

Hello,

thanks for the suggestions.

Pre commit hooks (e.g. black, isort). For example see below:

that does not work out of the box, no? I mean you need to set it up locally with an additional package, no?
we have makefile commands to ease the dev in case you didn't see them: make format

Introduce poetry https://python-poetry.org/ it's a really good dependency and packaging manager that replaces the setup.py script with its own automatic virtual environment handling. See an example toml spec below:

I know poetry but I'm not sure to understand the advantages over a setup.py + setup.cfg (as you need to have a setup.py anyway). What makes it much better compared to a built-in setup.py?
(poetry also comes at a cost of an additional dependency :/)

Add a script, something like setup_dev.sh, which can be run to setup development environment. See below for an example:

isn't the following easy enough? (from the doc https://stable-baselines3.readthedocs.io/en/master/guide/install.html#development-version)

git clone https://github.com/DLR-RM/stable-baselines3 && cd stable-baselines3
pip install -e .[docs,tests,extra]

@0xprofessooor
Copy link
Contributor Author

Yes, you need to use pre-commit package to add the pre-commit hooks. However, in doing so you could also remove the black isort dependencies etc. since these would be defined in the yaml config file which links directly to the git repos. make format is fine, but it's also an extra line to enter every commit and/or pull request, it's nice not to have to worry about it at all and if for some reason you want to revert to a previous commit you don't have to worry about the format of that commit.

As far as I understand it, poetry completely replaces setup.py (all the dev package instructions can also be defined in poetry.toml). I think it's worth it since you don't need to worry about setting up/handling the virtual environment at all (unless you want to) and the separation of the poetry.lock file is useful for rigid dependency handling (poetry.toml and poetry.lock are separate files, the lock file is where the dependencies are installed from, but you should only edit the toml file). However, the main advantage comes from publishing version releases; it can now be done in two lines with poetry build and poetry publish! https://python-poetry.org/docs/libraries/

The pip install -e .[docs,tests,extra] command requires the setup.py script, which could no longer exist if poetry were used. However, it's equally easy to setup the dev environment with poetry using poetry install which will set up the virtual environment and download all the dependencies as well as resolving all the versioning.

@araffin
Copy link
Member

araffin commented Sep 4, 2021

Yes, you need to use pre-commit package to add the pre-commit hooks.
However, in doing so you could also remove the black isort dependencies etc. since these would be defined in the yaml config file which links directly to the git repos.

Well, the dependency will still be there, just hidden.

make format is fine, but it's also an extra line to enter every commit and/or pull request,

That's true but it's really fast (faster than a git add .) and pre-commit would require extra configurations steps for every user that would like to contribute (vs a pip install -e .[tests]).

As far as I understand it, poetry completely replaces setup.py

So you cannot use pip anymore to install the package?
(That would break a lot of code relying on SB3 master version, including notebooks...)

I think it's worth it since you don't need to worry about setting up/handling the virtual environment at all

Oh, poetry is meant to have one virtual env per package?

poetry.lock file is useful for rigid dependency handling

I understand the importance of the lock file, but that's where conda env export work equally well I would say.

However, the main advantage comes from publishing version releases; it can now be done in two lines with poetry build and poetry publish

I think that make sense for more complicated package (that require building assets), in our case, we have make release that does the job ;)

@araffin araffin removed the Maintainers on vacation Maintainers are on vacation so they can recharge their batteries, we will be back soon ;) label Sep 4, 2021
@0xprofessooor
Copy link
Contributor Author

Yeah fair enough I think perhaps it isn't worth the effort of switching over and getting used to a new system given the current implementation works enough, I'll close the issue :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants