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

[Train] Clarify Session.get_next docstring #20877

Merged
merged 4 commits into from
Dec 7, 2021

Conversation

bveeramani
Copy link
Member

@bveeramani bveeramani commented Dec 3, 2021

Why are these changes needed?

This PR addresses two issues an issue with the Session.get_next docstring:

  • The docstring summary is in the wrong tense. It's unclear whether the tense should be imperative or non-imperative. The Ray documentation states that we use Google style (which is non-imperative), but we are formatting using PEP8 (which is imperative). Moreover, we use both imperative and non-imperative summaries across the Ray Train code. I've stuck with a non-imperative summary for consistency with the rest of the Session class.
  • The docstring doesn't describe the conditions under which the function returns None.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@bveeramani bveeramani added python Pull requests that update Python code train Ray Train Related Issue labels Dec 3, 2021
@bveeramani bveeramani self-assigned this Dec 3, 2021
@bveeramani bveeramani changed the title Clarify get_next docstring [Train] Clarify Session.get_next docstring Dec 3, 2021
@bveeramani bveeramani assigned amogkam and matthewdeng and unassigned bveeramani Dec 3, 2021
"""Gets next result from the queue."""
"""Get the next ``TrainingResult`` from the result queue.

If the result queue is empty, then this function returns None.
Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't sure whether to put this information under Returns: or under the docstring summary. I don't think it really matters, though; I may just be bike-shedding.

@bveeramani bveeramani marked this pull request as draft December 3, 2021 22:46
@bveeramani bveeramani marked this pull request as ready for review December 3, 2021 22:54
@amogkam amogkam merged commit 17b6130 into ray-project:master Dec 7, 2021
@bveeramani bveeramani deleted the get-next-docstring branch December 7, 2021 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code train Ray Train Related Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants