Basic Continuous Integration using Tox and GitHub Actions

awslabs / mlmax

Example templates for the delivery of custom ML solutions to production so you can get started quickly without having to make too many design choices.

https://mlmax.readthedocs.io/en/latest/

Apache License 2.0

66 stars 19 forks source link

Basic Continuous Integration using Tox and GitHub Actions #46

Closed josiahdavis closed 3 years ago

josiahdavis commented 3 years ago

Issue #, if available: NA

Description of changes:

Basic continuous integration using Tox and GitHub Actions

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

josiahdavis commented 3 years ago

@verdimrc I'd like to get your feedback on the approach here for a GH Actions + tox based workflow. Not quite ready to merge it yet, might make 1-2 minor changes to the tox.ini file, just looking for a second set of eyes on it.

verdimrc commented 3 years ago

Minor comments on tox.ini:

under [pytest], remove --ignore=test/s3fscompat (this was for another test that doesn't exist in this repo).
pydocstyle: is the plan to enable this step in the actions? In my experience, its default setting might feel a bit too stringent to some, esp. for a stylistic / documentation aspect rather than for code correctness.

verdimrc commented 3 years ago

To remove [testenv:reformat] in tox.ini, as this won't be used in the actions.

verdimrc commented 3 years ago

Suggest a [testenv:isort-check].

josiahdavis commented 3 years ago

To remove [testenv:reformat] in tox.ini, as this won't be used in the actions.

Do you think it might be useful to keep it in there as a handy utility for folks who want to use it locally though?

verdimrc commented 3 years ago

To remove [testenv:reformat] in tox.ini, as this won't be used in the actions.

Do you think it might be useful to keep it in there as a handy utility for folks who want to use it locally though?

pre-commit handles this case. (and fyi, that reformat entry predates precommit, so in a way it's a remnant from the past).

I see some benefits for those who want to use it, so maybe put some commentaries is a good middle ground?

josiahdavis commented 3 years ago

Minor comments on tox.ini:

under [pytest], remove --ignore=test/s3fscompat (this was for another test that doesn't exist in this repo).

Yes, I should have removed this one.

pydocstyle: is the plan to enable this step in the actions? In my experience, its default setting might feel a bit too stringent to some, esp. for a stylistic / documentation aspect rather than for code correctness.

Agreed it does feel too stringent, and I was not planning to put it into the CI. That said, on the fence about whether to leave it in their as on optional check or no.