Skip to content
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 Callbacks doc #1630

Merged
merged 8 commits into from
May 20, 2021
Merged

add Callbacks doc #1630

merged 8 commits into from
May 20, 2021

Conversation

jieru-hu
Copy link
Contributor

Closes #1526

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 20, 2021
@jieru-hu jieru-hu requested review from omry and Jasha10 May 20, 2021 00:55
Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions:

website/docs/experimental/callbacks.md Outdated Show resolved Hide resolved
website/docs/experimental/callbacks.md Outdated Show resolved Hide resolved
website/docs/experimental/callbacks.md Outdated Show resolved Hide resolved
website/docs/experimental/callbacks.md Outdated Show resolved Hide resolved
@jieru-hu
Copy link
Contributor Author

Thanks @Jasha10 for the great edits. I applied your suggestions and made some minor changes on top :)

Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, looks good to me! Just one more minor edit:

website/docs/experimental/callbacks.md Outdated Show resolved Hide resolved
jieru-hu and others added 8 commits May 20, 2021 11:09
Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>
Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>
Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>
Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>
Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>
@jieru-hu jieru-hu merged commit 090a9bd into facebookresearch:master May 20, 2021
@jieru-hu jieru-hu deleted the 1526 branch May 20, 2021 19:14
Comment on lines +73 to +76
Say we have `MyCallback` so after every job ends we can upload a certain file to a S3 bucket.
For simplicity we include this Callback class within the application, in real life you should have the
Callback in a separate file.
Running the application, we can see our custom method `on_job_end` was called.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence does not read right to me.
Something like this would work better in my opinion:

In this example, we will use a callback to upload a certain file to an S3 bucket at the end of the job.
For simplicity we include this Callback class within the application, real applications are likely to place it in a different Python file.
Running the application, we can see our custom method on_job_end was called.

```
</details>

### Configure Callback
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would work better:

### Callback usage example

</div>


### Callback ordering
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

### Callback execution order

Comment on lines +166 to +185
The `on_run_start` or `on_multirun_start` method will get called first,
followed by `on_job_start` (called once for each job).
After each job `on_job_end` is called, and finally either `on_run_end` or
`on_multirun_end` is called one time before the application exits.

In the `hydra.callbacks` section of your config, you can use a list to register multiple callbacks. They will be called in the final composed order for `start` events and
in reversed order for `end` events. So, for example, suppose we have the following composed config:
```commandline title="python my_app.py --cfg hydra -p hydra.callbacks"
# @package hydra.callbacks
my_callback1:
_target_: my_app.MyCallback1
param1: val1
my_callback2:
_target_: my_app.MyCallback2
param2: val2
```
Before each job starts, `MyCallback1.on_job_start` will get called first,
followed by `MyCallback2.on_job_start`.
After each job ends, `MyCallback2.on_job_end` will get called first,
followed by `MyCallback1.on_job_end`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of text to explain something pretty simple:

start events are getting called in order defined in the config and end events are getting called in reverse order.

This can probably be simplified quite a bit.

@jieru-hu
Copy link
Contributor Author

Thanks for taking a look omry, i've created #1636 to address the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document hydra.callbacks api.
4 participants