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

chore: adding more tests to ActivationsStore + light refactoring #20

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

chanind
Copy link
Collaborator

@chanind chanind commented Mar 6, 2024

This PR contains a few new tests for ActivationsStore (pretty modest though), and some light refactoring of the test setup code and ActivationsStore. Specifically, this PR changes the following:

  • Adds tests/helpers.py where generic test utils can be put
  • Moves the set of the LanguageModelSAERunnerConfig to build_sae_config helper in tests/helpers.py. This helper defines a default config, but allows any overrides to be passed in as kwargs. This is trying to be a lightweight version of the test factory pattern, like in https://github.com/FactoryBoy/factory_boy. This lets tests just override only the keys that are important for the test, and makes it more clear what part of the config is relevant for the test.
  • Fixes a minor bug in ActivationsStore where sometimes the BOS token is added twice at the start of a batch
  • Simplifies some duplicated code in ActivationsStore, and breaks out some of the token selecting logic into a private function for easier testing.
  • Allows passing a dataset as an explicit param to ActivationStore constructor, to allow for testing with a small dataset that the individual tests can control and make assertions against.

Happy to change or remove any of this if it's not what's desired!

@jbloomAus
Copy link
Owner

Hey David, looks great! I'd love to experiment with doing benchmark runs before/after changes. Do you think you might be able to do like a tiny-stories training run with / without these changes so we can see the difference? Happy to help tune hyperparameters.

@chanind
Copy link
Collaborator Author

chanind commented Mar 7, 2024

Yeah, definitely, I'll give that a try. Maybe we can get a snapshot test or something similar set up on a small dataset which can run automatically in CI and give confidence that the overall system is working. I see there's benchmark tests that have the shell of something like that, but are missing assertions that would give confidence in the overall results. I'll work on setting up something like that and reach out when there's something we can look over

@jbloomAus
Copy link
Owner

@chanind How's this going? Just checking in.

@chanind
Copy link
Collaborator Author

chanind commented Mar 10, 2024

Apologies for being MIA on this! I've just been busy with other projects the past few days, but I should be able make progress on this this week.

@jbloomAus jbloomAus merged commit 69dcf8e into jbloomAus:main Mar 21, 2024
2 checks passed
tom-pollak pushed a commit to tom-pollak/SAELens that referenced this pull request Oct 22, 2024
chore: adding more tests to ActivationsStore + light refactoring
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.

2 participants