-
Notifications
You must be signed in to change notification settings - Fork 331
Add a contributing guide #67
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
Changes from 2 commits
79c1bbc
430d634
037fdde
99094e5
1a9703f
c83a94e
3347da2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| ## How to contribute code | ||
|
|
||
| Follow these steps to submit your code contribution. | ||
|
|
||
| ### Step 1. Open an issue | ||
|
|
||
| Before making any changes, we recommend opening an issue (if one doesn't already | ||
| exist) and discussing your proposed changes. This way, we can give you feedback | ||
| and validate the proposed changes. | ||
|
|
||
| If the changes are minor (simple bug fix or documentation fix), then feel free | ||
| to open a PR without discussion. | ||
|
|
||
| ### Step 2. Make code changes | ||
|
|
||
| To make code changes, you need to fork the repository. You will need to setup a | ||
LukeWood marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| development environment and run the unit tests. This is covered in section | ||
| "Setup environment". | ||
|
|
||
| ### Step 3. Create a pull request | ||
|
|
||
| Once the change is ready, open a pull request from your branch in your fork to | ||
| the master branch in [keras-team/keras](https://github.com/keras-team/keras). | ||
LukeWood marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ### Step 4. Sign the Contributor License Agreement | ||
|
|
||
| After creating the pull request, the `google-cla` bot will comment on your pull | ||
LukeWood marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| request with instructions on signing the Contributor License Agreement (CLA) if | ||
| you haven't done so. Please follow the instructions to sign the CLA. A `cla:yes` | ||
| tag is then added to the pull request. | ||
|
|
||
|  | ||
|
|
||
|
|
||
| ### Step 5. Code review | ||
|
|
||
| CI tests will automatically be run directly on your pull request. Their | ||
| status will be reported back via GitHub actions. | ||
|
|
||
| There may be | ||
| several rounds of comments and code changes before the pull request gets | ||
| approved by the reviewer. | ||
|
|
||
|  | ||
|
|
||
| ### Step 6. Merging | ||
|
|
||
| Once the pull request is approved, a team member will take care of merging. | ||
|
|
||
| ## Setup environment | ||
|
|
||
| Setting up your KerasCV development environment is quite easy. You simply | ||
| need to run the following commands: | ||
|
|
||
| ```shell | ||
| git clone https://github.com/YOUR_GITHUB_USERNAME/keras-cv.git | ||
LukeWood marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| cd keras-cv | ||
| pip install ".[tests]" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: So, installing: will install the list on line 30. WDYT?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I personally am a fan of the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest using |
||
| python setup.py develop | ||
| ``` | ||
|
|
||
| Following these commands you should be able to run the tests using `pytest keras_cv`. | ||
| Please report any issues running tests following these steps. | ||
|
|
||
| ## Run tests | ||
|
|
||
| KerasCV is tested using [PyTest](https://docs.pytest.org/en/6.2.x/). | ||
|
|
||
| ### Run a test file | ||
|
|
||
| To run a test file, run `pytest path/to/file` from the root directory of keras\_cv. | ||
|
|
||
| ### Run a single test case | ||
|
|
||
| To run a single test, you can use `-k=<your_regex>` | ||
| to use regular expression to match the test you want to run. For example, you | ||
| can use the following command to run all the tests in `cut_mix_test.py`, | ||
| whose names contain `label`, | ||
|
|
||
| ``` | ||
| pytest keras_cv/layers/preprocessing/cut_mix_test.py -k="label" | ||
| ``` | ||
|
|
||
| ### Run all tests | ||
|
|
||
| Running all of the tests in KerasCV is as easy as: | ||
LukeWood marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ``` | ||
| pytest keras_cv/ | ||
| ``` | ||
LukeWood marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### Formatting the Code | ||
| In order to format the code you can use the `shell/format.sh` script. | ||
LukeWood marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| If this does not resolve the issue, try updating `isort` and `black` | ||
| via `pip install --upgrade black` and `pip install --upgrade isort`. | ||
LukeWood marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
Uh oh!
There was an error while loading. Please reload this page.