use py-template by dtrifiro · Pull Request #281 · treeverse/dvclive

dtrifiro · 2022-09-01T09:56:02Z

Use cruft/cookiecutter template from https://github.com/iterative/py-template

closes #272

dtrifiro · 2022-09-01T11:22:40Z

Ended up adding this because codespell was complaining:

src/dvclive/data/plot.py:58: fpr ==> for, far, fps src/dvclive/data/plot.py:68: fpr ==> for, far, fps src/dvclive/data/plot.py:73: fpr ==> for, far, fps src/dvclive/data/plot.py:74: fpr ==> for, far, fps src/dvclive/data/plot.py:111: fpr ==> for, far, fps src/dvclive/data/plot.py:121: fpr ==> for, far, fps src/dvclive/data/plot.py:127: fpr ==> for, far, fps src/dvclive/data/plot.py:128: fpr ==> for, far, fps

I usually add this to .pre-commit-config.yaml, to avoid adding more config to setup.cfg.

https://github.com/iterative/scmrepo/blob/bbcf8f337cf3ac33764c2c7fc227e8e7bd403cea/.pre-commit-config.yaml#L30

I see that there is a PR for pyproject.toml support: codespell-project/codespell#2290.

It's good for now.

skshetry · 2022-09-01T14:35:44Z

I will try to review it by tomorrow. 🙂

pared

Some questions:

Are most of the changes automatically generated?
a.) I see a lot of mentions of my name, and while I am flattered, I think there should be more Iterative and less Paweł Redzyński :D
b.) Do we need this huge .gitignore? There are rules for cython, django and so on while I am not sure there is a chance for stumbling upon them while developing this package.

skshetry · 2022-09-02T15:19:01Z

b.) Do we need this huge .gitignore? There are rules for cython, django and so on while I am not sure there is a chance for stumbling upon them while developing this package.

I can answer this question. It's a template from https://github.com/github/gitignore, and it has made all the decisions for us regarding useful gitignores required for a Python library/application. It's true that we might not use these libraries, but we might use them on some projects, it's one py-template satisfying all of our projects' needs.
The ignores in that file is reasonable, and we are unlikely to ever be in conflict.

Adding some entries to the .gitignore in the future, even though it improves your quality of life, is going to be difficult, with potential pushbacks or suggestions to work around. Eg: In dvc, we didn't have venv for so long, and proposing one would surely get pushback, suggestions to just use env or to use .git/info/exclude. It's better to start with a standard template of .gitignore, I think. Tbh I don't see any harm with huge .gitignore.

I'm happy to discuss more and open to change. I'll suggest opening an issue on py-template if you disagree and we can reach a consensus. 🙂

skshetry · 2022-09-02T15:22:07Z

@dtrifiro, what about the order, do they match with py-template? That's the only thing I want to review here. 😄

Otherwise, update-template may keep on raising conflicts.

pared · 2022-09-02T16:18:50Z

It was there before, so I left it 🙂, I think it should stay, unless you want it removed?

I don't mind it, just don't want to take credit for our collective work :)
However, I would probably change to Iterative in the License, as it seems that Iterative is a License provider, even if I am an author

skshetry · 2022-09-05T08:04:33Z

Did dvclive provide pre-commit hooks?

Not sure what you mean here. I think this can be safely replaced with .pre-commit-config.yaml

skshetry · 2022-09-05T08:07:16Z

Does dvclive need to provide version info?

>>> from importlib.metadata import version; version('dvclive')

Users can get this with above ^.

Got rid of it, as well as getting rid of src/dvclive/version.py

skshetry

🔥

codecov-commenter · 2022-09-05T08:26:56Z

Codecov Report

Base: 90.67% // Head: 93.43% // Increases project coverage by +2.75% 🎉

Coverage data is based on head (a426332) compared to base (ca3dc22).
Patch coverage: 96.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #281      +/-   ##
==========================================
+ Coverage   90.67%   93.43%   +2.75%     
==========================================
  Files          21       35      +14     
  Lines         751     1554     +803     
  Branches        0      169     +169     
==========================================
+ Hits          681     1452     +771     
- Misses         70       80      +10     
- Partials        0       22      +22

Impacted Files	Coverage Δ
src/dvclive/__init__.py	`100.00% <ø> (ø)`
src/dvclive/catalyst.py	`100.00% <ø> (ø)`
src/dvclive/data/image.py	`88.88% <ø> (ø)`
src/dvclive/data/utils.py	`75.00% <ø> (ø)`
src/dvclive/dvc.py	`19.35% <0.00%> (ø)`
src/dvclive/env.py	`100.00% <ø> (ø)`
src/dvclive/error.py	`86.95% <ø> (ø)`
src/dvclive/fastai.py	`92.85% <ø> (ø)`
src/dvclive/huggingface.py	`100.00% <ø> (ø)`
src/dvclive/keras.py	`100.00% <ø> (ø)`
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dtrifiro · 2022-09-07T10:29:04Z

With the lastest push I got rid of src/dvclive/{_,}version.py and updated pyproject.toml accordingly. Will merge by the end of the day if nobody has any other comments.

daavoo

Haven't really reviewed in-depth, but looks good on high level so let's merge and follow up if needed

pared

LGTM, though as I mentioned, I think we should change in the LICENSE to Iterative, from Paweł Redzyński. I am the author, but I believe its Iterative who provides the license to use, as I created it under Iterative's name.

Most ML frameworks won't work on pypy and won't be available on 3.11 until it is officially released

Co-authored-by: skshetry <suagatchhetri@outlook.com>

dtrifiro · 2022-09-08T15:05:23Z

Thanks everybody!

dtrifiro requested review from daavoo, pared and skshetry and removed request for pared and skshetry September 1, 2022 09:56

dtrifiro marked this pull request as ready for review September 1, 2022 09:57

dtrifiro marked this pull request as draft September 1, 2022 10:01

dtrifiro force-pushed the use-py-template branch from f000b1e to dd66378 Compare September 1, 2022 10:16

skshetry reviewed Sep 1, 2022

View reviewed changes

Comment thread .cruft.json Outdated

skshetry reviewed Sep 1, 2022

View reviewed changes

Comment thread .cruft.json Outdated

skshetry reviewed Sep 1, 2022

View reviewed changes

Comment thread .cruft.json Outdated

skshetry reviewed Sep 1, 2022

View reviewed changes

Comment thread setup.cfg Outdated

skshetry reviewed Sep 1, 2022

View reviewed changes

Comment thread setup.cfg Outdated

skshetry reviewed Sep 1, 2022

View reviewed changes

Comment thread setup.cfg Outdated

dtrifiro force-pushed the use-py-template branch 2 times, most recently from e181bb6 to ecc74d3 Compare September 1, 2022 10:37

dtrifiro marked this pull request as ready for review September 1, 2022 10:37

dtrifiro force-pushed the use-py-template branch from ecc74d3 to 4cddf39 Compare September 1, 2022 10:40

This was referenced Sep 1, 2022

use setuptools_scm #278

Closed

gha: tests with pypy #260

Closed

dtrifiro commented Sep 1, 2022

View reviewed changes

dtrifiro force-pushed the use-py-template branch from f2d238e to 96bf5d8 Compare September 1, 2022 12:58

pared reviewed Sep 2, 2022

View reviewed changes

skshetry reviewed Sep 5, 2022

View reviewed changes

Comment thread pyproject.toml Outdated

skshetry reviewed Sep 5, 2022

View reviewed changes

Comment thread setup.cfg Outdated

skshetry reviewed Sep 5, 2022

View reviewed changes

Comment thread setup.cfg Outdated

skshetry reviewed Sep 5, 2022

View reviewed changes

Comment thread setup.cfg Outdated

skshetry approved these changes Sep 5, 2022

View reviewed changes

dtrifiro force-pushed the use-py-template branch from a426332 to 119c295 Compare September 7, 2022 10:26

daavoo approved these changes Sep 7, 2022

View reviewed changes

skshetry reviewed Sep 7, 2022

View reviewed changes

Comment thread .gitignore Outdated

pared suggested changes Sep 7, 2022

View reviewed changes

Daniele Trifirò added 7 commits September 7, 2022 13:51

use py-template

61355a8

fix type hints/mypy errors

1890643

fix typos

a7f3689

utils: fix types in open_file_in_browser

4e5d77d

explicitly specify encoding in open() calls

2d9b2b3

gha: increase timeout

a9b82a8

gha: don't test on pypy or 3.11

067ac36

Most ML frameworks won't work on pypy and won't be available on 3.11 until it is officially released

dtrifiro force-pushed the use-py-template branch from 119c295 to 067ac36 Compare September 7, 2022 11:51

dtrifiro requested a review from pared September 7, 2022 11:53

skshetry reviewed Sep 7, 2022

View reviewed changes

Comment thread setup.cfg Outdated

pared approved these changes Sep 8, 2022

View reviewed changes

setup.cfg: set authors to Iterative

13e30fd

Co-authored-by: skshetry <suagatchhetri@outlook.com>

dtrifiro merged commit d658bd6 into treeverse:main Sep 8, 2022

dtrifiro deleted the use-py-template branch September 8, 2022 15:05

Conversation

dtrifiro commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dtrifiro Sep 1, 2022

Choose a reason for hiding this comment

Uh oh!

skshetry Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

skshetry commented Sep 1, 2022

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

skshetry commented Sep 2, 2022

Uh oh!

skshetry commented Sep 2, 2022

Uh oh!

pared commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skshetry Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

dtrifiro Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

skshetry Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

dtrifiro Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skshetry left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dtrifiro commented Sep 7, 2022

Uh oh!

daavoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dtrifiro commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dtrifiro commented Sep 1, 2022 •

edited

Loading

pared commented Sep 2, 2022 •

edited

Loading

codecov-commenter commented Sep 5, 2022 •

edited

Loading