Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Introduce poetry #182

Merged
merged 16 commits into from
May 19, 2021
Merged

Introduce poetry #182

merged 16 commits into from
May 19, 2021

Conversation

kenoss
Copy link
Contributor

@kenoss kenoss commented Mar 31, 2021

This PR is a starting point and maybe not complete yet. Let me hear your opinion.

@@ -0,0 +1,23 @@
[tool.poetry]
name = "handyrl"
version = "0.0.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the real version. Fill it.

To use games of kaggle environments (e.g. Hungry Geese) you can install also additional dependencies.
```
pip3 install -r handyrl/envs/kaggle/requirements.txt
poetry install
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, handyrl is not a library yet and intended to use with environments under env/. And, this is a framework of training. So we don't need to care about docker and additional dependency poetry.

Additional option is:

  1. Provide both pyproject.toml and requirements.txt.
  2. Check coherence of them in CI by using https://python-poetry.org/docs/cli/#export

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#182 (comment)

We provide requirements.txt until #183 .

@kenoss kenoss marked this pull request as ready for review March 31, 2021 15:58
@kenoss
Copy link
Contributor Author

kenoss commented Mar 31, 2021

@ikki407 @YuriCat Could you assign reviewers? Thanks.

@kenoss kenoss mentioned this pull request Mar 31, 2021
@ikki407 ikki407 self-requested a review March 31, 2021 17:06
@ikki407
Copy link
Member

ikki407 commented Mar 31, 2021

First of all, we’d like to thank you for your PR and suggestions! We will look into the updates soon. Then let’s discuss it. Thanks!

@YuriCat and me are assigned as the reviewers.

@ikki407
Copy link
Member

ikki407 commented Apr 13, 2021

Sorry for the late reply. Here is our brief opinion.

  1. We will make HandyRL available on an import-based way (e.g. feature: import based enabled #175)
  2. HandyRL should be made into a library, but we think the library will/should provide only the minimum functions such as naive self-play, customized environments and networks.
  3. As discussed in the issue comment, we'd like to have an aspect as a customizable tool since it is impossible to support all RL settings... Thus, we'd like to keep the following method as one of our recommended method: git clone/fork -> copy/customize the files -> train/evaluate with it.

How about doing this?

  • Making a library is done concisely with Poetry
  • As the usage of HandyRL, there will keep two methods through pip install and git clone

And, can we run the command without poetry run even if using poetry?

poetry run python main.py --train
↓
python main.py --train  # Can we do this?

@kenoss
Copy link
Contributor Author

kenoss commented Apr 13, 2021

we'd like to have an aspect as a customizable tool since it is impossible to support all RL settings...

I see your point.
In that case, I personally prefer the following usage:

  1. Download HandyRL to the local machine (with git or zip or anything.)
  2. Modify HandyRL.
  3. Training code loads the variant by path-dependency.

Skilled users may prefer to fork this repository instead of the above steps 1 and 2 because it is an easier way to manage diffs. (And, they can contribute to upstream (here) if their modification is useful.)

And, can we run the command without poetry run even if using poetry?

If #183 is complete and the user has installed HandyRL in the environment, yes.
(My modification of README.md in this PR is based on the absence of #183 nor #175.)
But I also think managing training code with some modern tool is easier. (README.md に上記の poetry のやり方だけ書いて過度に poetry 推しをする必要はないですが. poetry の話しかしてないのは最近僕が poetry しか使ってなくて他のツールでどう書くかあんまり知らないという理由によるものです.)

@ikki407
Copy link
Member

ikki407 commented Apr 15, 2021

Skilled users may prefer to fork this repository instead of the above steps 1 and 2 because it is an easier way to manage diffs. (And, they can contribute to upstream (here) if their modification is useful.)

That's right. I believe that (public/private) fork is the better way if the user use HandyRL for solving their problems.
path-dependency is nice! But I prefer the simple fork :)
If the user wants to use your suggestion usage (1. Download...), I feel like we can leave it to the user.

If #183 is complete and the user has installed HandyRL in the environment, yes.
(My modification of README.md in this PR is based on the absence of #183 nor #175.)
But I also think managing training code with some modern tool is easier.

Got it! I agree with it unless it's not too difficult for beginners in reinforcement learning or programming.
At this moment, I think, the integration of Poetry will mainly affect the owner and contributors of HandyRL. So, I'm positive about using Poetry.

@ikki407
Copy link
Member

ikki407 commented Apr 15, 2021

In summary as my opinion, the following actions may be considered as a future situation.

  1. Introduce poetry to publish to pypi and install library
    • publish to pypi by repository owners
    • users run pip install -> import handyrl -> train/evaluate
  2. Also keep current usage in README
    • git clone / download zip / copy script -> naive pip install / poetry install -> scripting codes -> train/evaluate
    • describe each ways briefly in readme (e.g., using
      tag ...)

@ikki407
Copy link
Member

ikki407 commented Apr 15, 2021

What do you think about this plan? @YuriCat @kenoss

@kenoss
Copy link
Contributor Author

kenoss commented Apr 16, 2021

Sounds good. I fixed the diff along with your plan.
I think it is ready for review. (If @YuriCat is OK.)

@kenoss
Copy link
Contributor Author

kenoss commented Apr 17, 2021

Note that poetry export generates requirements.txt that is equivalent to lockfile. It is not ideal for users but I think it is a temporary measure and acceptable.

@ikki407
Copy link
Member

ikki407 commented Apr 17, 2021

Thank you very much! I’ll review the changes and try it on my machine.

As another topic, I think it is necessary to make docstrings in important classes and functions when making a library (maybe type hints too...?). I think we (me or YuriCat) should do this after the implementation is fixed.

@kenoss
Copy link
Contributor Author

kenoss commented Apr 17, 2021

Documentation and type hints are nice!
It's better to manage rest works in #183 .

@kenoss kenoss mentioned this pull request Apr 17, 2021
@YuriCat
Copy link
Contributor

YuriCat commented May 1, 2021

@kenoss
Hi, how do you think about using poetry without poetry.lock?

@kenoss
Copy link
Contributor Author

kenoss commented May 2, 2021

Assume that

Then, the pros of having a lockfile are:

  • We, including developers and users, can share "what environment can run it correctly" more precisely and declaratively.
  • We can test with older dependencies. See https://blog.cardina1.red/2020/12/14/dont-leave-cargo-toml-broken/ . AFAIK, there's no feature of poetry that "tests with oldest dependencies". Lockfile provides a compromised version.

There are no cons, I think.

This PR does not contain test-with-up-to-date-dependencies. Shall I add?

@kenoss
Copy link
Contributor Author

kenoss commented May 2, 2021

Note that, if the target is an application, we should have a lockfile.

@YuriCat
Copy link
Contributor

YuriCat commented May 4, 2021

Thanks!
I understand and believe your opinion is totally correct, while poetry.lock looks just too long and there seems to be unnecessary information, e.g. information about Python2.

@kenoss
Copy link
Contributor Author

kenoss commented May 5, 2021

Like this line?
IIUC, this comes from Requires-Python field of package metadata of each package and poetry "cache" to its lockifle to make resolving fast. You shouldn't care about it.

@kenoss
Copy link
Contributor Author

kenoss commented May 6, 2021

Added test with up-to-date dependencies.

@YuriCat Could you re-review it, please?

Copy link
Member

@ikki407 ikki407 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kenoss, sorry for late approval. We're merging this great PR into master branch at the end of this month. Thank you!

@kenoss
Copy link
Contributor Author

kenoss commented May 19, 2021

Thanks for the review!
But I can't see the merge button. (I guess this is by the config of this repository.) Cloud you merge it, please.

@ikki407
Copy link
Member

ikki407 commented May 19, 2021

Sure, the administrators only merge PR. I will merge it now! 🚀

@ikki407 ikki407 merged commit 025c984 into DeNA:develop May 19, 2021
@kenoss kenoss deleted the introduce-poetry branch May 21, 2021 02:30
@kenoss
Copy link
Contributor Author

kenoss commented May 21, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants