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

remove project path from model hash #830

Merged
merged 1 commit into from
Aug 26, 2021
Merged

remove project path from model hash #830

merged 1 commit into from
Aug 26, 2021

Conversation

shaycrk
Copy link
Contributor

@shaycrk shaycrk commented Mar 6, 2021

Looks like this should be all we have to do to remove the project path from the model hash, but we may want to bump the version when brining this in since the model hashes won't be backwards-compatible with runs from current versions of triage.

@shaycrk shaycrk requested a review from thcrock March 6, 2021 04:40
@shaycrk
Copy link
Contributor Author

shaycrk commented Mar 6, 2021

For what it's worth, if we really wanted to provide backwards compatibility, the two options I can think of would be:

  • inspect the models table for a matching model and use the existing hash (if we might already be writing something similar for random numbers, could take advantage of that code)
  • pass a new use_legacy_hash parameter here (however, _model_hash() is only used pretty deep down the chain, so this would require a lot of changes, and if the parameter were passed via experiment config, we'd also want to be careful that it doesn't change the experiment hash)

Copy link
Contributor

@thcrock thcrock left a comment

Choose a reason for hiding this comment

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

The only thing I would add is a note somewhere, maybe in documentation, about the lack of backwards compatibility. It may be worth tagging a new major version as well, after this is merged.

@thcrock
Copy link
Contributor

thcrock commented Mar 8, 2021

It's probably also worth doing at least the YAML upgrade to silence the security message before tagging the major version.

It's probably not necessary to tag a major version right after this, but rather I'd make the next version, whenever it comes, major instead of minor.

@shaycrk
Copy link
Contributor Author

shaycrk commented Mar 19, 2021

Was going to drop something in the docs here, but realized I couldn't find a really clear place to do so (unless I missed it, of course). Aside from the release notes when we update the version, is there somewhere we've generally documented changes that break backward compatibility?

@shaycrk shaycrk merged commit 0b18c79 into master Aug 26, 2021
@shaycrk shaycrk deleted the kit_model_hash branch August 26, 2021 21:16
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