Pin merlin-core version to specific commit to avoid breaking changes
#199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are some upcoming changes to the package names in `merlin-core` (e.g. `graph->dag`) that would break the tests in `merlin-models` if we continued depending directly on the latest commit in `merlin-core`. Pinning the version to a specific commit allows us to bump the version here in the same PR that propagates upstream changes from `merlin-core` without breaking the tests/CI on everyone else's PRs.
|
Are there other changes aside from the merlin.graph -> merlin.dag coming in merlin-core that will cause breaks here? merlin models doesn't depend on merlin.graph at all, so I'm wondering if we can just leave pinned to the latest main |
|
I was thinking of NVIDIA-Merlin/core#4 and NVIDIA-Merlin/core#5. We can deal with those changes when they happen, I guess? |
|
@benfred @karlhigley Since we’re cutting a release this week, should we hold off on this and pin merlin-core to the right version (through pip)? |
|
Yeah, the delay getting it merged has made this PR pointless, so closing it |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goals ⚽
There are some upcoming changes to the package names in
merlin-core(e.g.graph->dag) that would break the tests inmerlin-modelsif we continued depending directly on the latest commit inmerlin-core. Pinning the version to a specific commit allows us to bump the version here in the same PR that propagates upstream changes frommerlin-corewithout breaking the tests/CI on everyone else's PRs.Implementation Details 🚧
Updated requirements.txt to reference a specific commit in
merlin-coreTesting Details 🔍
Covered by existing tests