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

Update docs to show prepend_bos and padding_side are optional #418

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

UFO-101
Copy link
Contributor

@UFO-101 UFO-101 commented Oct 16, 2023

Description

  • Improve documentation for prepend_bos and padding_side in several functions in HookedTransformer and utils. It is currently unclear that they are just intended to be used as overrides of the default values set in the initial config.
  • Improve the syntax of some type hints.

Type of change

  • This change requires a documentation update

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have not rewritten tests relating to key interfaces which would affect backward compatibility

Copy link
Collaborator

@alan-cooney alan-cooney left a comment

Choose a reason for hiding this comment

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

I think a better approach may be to just make this clear in the docstring, so that we don't have to release a breaking change. As long as the title is e.g. "Override Default for Prepend BOS" it's fairly clear.

The other changes seem good but mind splitting into separate PRs so we can keep the commit history clean?

@UFO-101 UFO-101 force-pushed the small-improvements branch 5 times, most recently from 0a720ac to b569265 Compare October 17, 2023 20:25
@UFO-101
Copy link
Contributor Author

UFO-101 commented Oct 17, 2023

@alan-cooney Undid parameter renaming and added / updated docstrings instead.

Moved other changes into new PRs #420 and #421

@UFO-101 UFO-101 force-pushed the small-improvements branch 2 times, most recently from e95639c to b807efa Compare October 17, 2023 20:37
…functions in HookedTransformer and utils. It is currently unclear that they are just intended to be used as overrides of the default values set in the initial config.

 - Improve the syntax of some type hints.
@neelnanda-io
Copy link
Collaborator

+1 to not rename the parameters, and just update the docs

@alan-cooney alan-cooney self-requested a review October 18, 2023 02:40
@alan-cooney alan-cooney changed the title Miscellaneous changes Update docs to show prepend_bos and padding_side are optional Oct 19, 2023
Copy link
Collaborator

@alan-cooney alan-cooney left a comment

Choose a reason for hiding this comment

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

We're going to add some more standards for docstrings shortly, but approving now and will make some tweaks once those are added.

@alan-cooney alan-cooney merged commit d3e3a10 into TransformerLensOrg:main Oct 19, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants