Add a contributing guide #67
Conversation
LukeWood
requested review from
fchollet,
qlzh727,
haifeng-jin and
mattdangerw
|
+haifeng and +matt as well - we will be reusing similar guides so it would be good to make sure I'm not missing anything here! |
|
We are trying to standardize a template for this file in the ecosystem at: It would be nice If you could contribute to the review and adopt it also in the Keras ecosystem |
|
Do you want to test all apis be compatible with all tf.distribute strategies? Do you want to build such testing infrastructure to help the community? |
ideally, but there may be some exceptions. Right now the COCORecall metric isn’t compatible with the TPU strategy. |
What would this look like? Basically a parameterized test case where a param is test strategy? I suppose it depends on if we can emulate distribution strategies without using underlying hardware |
| ```shell | ||
| git clone https://github.com/YOUR_GITHUB_USERNAME/keras.git | ||
| cd keras | ||
| pip install ".[tests]" |
There was a problem hiding this comment.
what is ".[test]"? Usually we list all the required dependency in a requirements.txt file, and use pip install -r requirements.txt.
There was a problem hiding this comment.
.[tests] installs everything required to run unit tests. You can see how this is handled in KerasTuner as well:
https://github.com/keras-team/keras-tuner/blob/master/setup.py#L40
So, installing:
pip install .
will install the list on line 30. pip install .[tests] will also install pytest, flake8, isort, etc.
WDYT?
There was a problem hiding this comment.
- @haifeng-jin + @mattdangerw to discuss best practices for managing dependencies as well.
I personally am a fan of the setup.py dependency management system.
There was a problem hiding this comment.
I am not sure! I don't really know enough about what is common practice in the python packaging world (or is this a legitimate source of contention?). It is nice to be able to split the testing and package requirements.
Some discussion:
https://stackoverflow.com/q/43658870
https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/
install_requires works and we have it now, so I am fine to leave as is unless we have a good reason to switch.
There was a problem hiding this comment.
Some extra context: the benefit of requirements is you can pin versions.
But at Google we are forced to update all dependencies anyways, so for Keras I think install_requires is superior.
There was a problem hiding this comment.
If requirements.txt makes it easier to pin a system installed tensorflow that might be a reason to use it. I am not sure, but for running cloud TPU jobs with the setup.py we have today, I've needed to hack it up to stop it from installing a new tf version that does not have proper TPU support. No need to resolve here, but that's something I will keep looking into.
There was a problem hiding this comment.
I would suggest using setup.py. One caveat is that the current setting for version number [Link] may throw an error when installing with pip install . because it imports the package to get the version number before installing anything, which may encounter some not installed packages. For example, the user would have to install tensorflow before running pip install .. It is not a big deal though. The user can tell which package to install from the error.
|
There are some existing testing utils in tensorflow/python/distribute for setting up various distribution strategies. Internally in google, the unit test can setup different hardware settings, eg multi-cpu/gpu/tpu and combinations, but it will be bit hard for any other local test env on user local. Only a few setting might work locally, eg mirror strategy with multi-cpu. |
Thanks for the reference. For the code style and linting, is there a standard formatter used by tf community? We would love to use that to avoid reinventing the wheel. |
I've not tried to do it in the template as it is already quite hard to just focus the community attention on a very minimal but common baseline of empty sections headlines. What I can tell you is that I believe in Just to take a look at some numbers on TF with few sample queries: But also with the small stats we have in Keras-cv check: |
| ```shell | ||
| git clone https://github.com/YOUR_GITHUB_USERNAME/keras.git | ||
| cd keras | ||
| pip install ".[tests]" |
There was a problem hiding this comment.
I would suggest using setup.py. One caveat is that the current setting for version number [Link] may throw an error when installing with pip install . because it imports the package to get the version number before installing anything, which may encounter some not installed packages. For example, the user would have to install tensorflow before running pip install .. It is not a big deal though. The user can tell which package to install from the error.
* Add a contributing guide * Address Scott's comments. * address Matt comments * update to use github cli * Update "Format the Code" header * Address introduction of tools comment * Update CONTRIBUTING.md
* Add a contributing guide * Address Scott's comments. * address Matt comments * update to use github cli * Update "Format the Code" header * Address introduction of tools comment * Update CONTRIBUTING.md
* Add log_cosh and huber loss * Docstring standardization * Format * Standardize wrapper function docstrings
We can eventually move this into governance if needed.