Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a contributing guide #67
Add a contributing guide #67
Changes from all commits
79c1bbc
430d634
037fdde
99094e5
1a9703f
c83a94e
3347da2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.[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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally am a fan of the
setup.py
dependency management system.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest using
setup.py
. One caveat is that the current setting for version number [Link] may throw an error when installing withpip 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 runningpip install .
. It is not a big deal though. The user can tell which package to install from the error.