-
Notifications
You must be signed in to change notification settings - Fork 2
Add entry_point plugin support for user defined HELM run_specs #42
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
Conversation
|
@Erotemic I guess we're stuck with failing tests until the boolq scenario is fixed upstream? |
|
We could:
|
|
Closing as the upstream PR has landed. |
|
Re-opening as the entrypoint updates here to plugin our custom run-specs/scenarios are still relevant. Removing the |
|
@Erotemic seems like CI doesn't like the URL dependency for helm, but I did confirm that our test pass locally. Do you think it makes more sense to pin to a particular commit (vs. the |
|
I think pinning to a branch makes sense. Frequent changes can be pushed to a branch other than It looks like git urls are not really supported in the project.dependencies section of pyproject.toml as they break pypi deployment. That means making a pypi package like kitware-helm might be the only way to have a truly robust install mechanism where the user doesn't have to think about it. https://docs.astral.sh/uv/pip/compatibility/?utm_source=chatgpt.com#transitive-url-dependencies To make CI pass so we can continue to develop I recommend making a This unblocks us while we think of a better way to handle this in general. (I think it will converge on a new kitware-helm package) |
|
I think I roughly follow you and the link is helpful, I've made some tweaks to that end (again mostly to get the CI passing). I do have reservations about maintaining dependencies it two distinct places (seems like a footgun). I'm with you that we'll likely converge on needing a PyPI |
| run: |- | ||
| python -m pip install pip uv -U | ||
| python -m uv pip install -r pyproject.toml --extra tests | ||
| python -m uv pip install -r requirements/runtime.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Erotemic is this the place (and only place) I needed to add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need it in the test_purepy_wheels section after python -m uv pip install --prerelease=allow "aiq-magnet[$INSTALL_EXTRAS]==$MOD_VERSION" -f wheelhouse. There are tow paths for testing the source dist and the wheel dist.
|
Alright so maybe the |
|
You should be correct. I'm taking a look. Tests that rely on network items will always have some sort of issue like this. I've got the issue reproduced locally. |
|
Ah, I see the issue. Upstream HELM removed the "latest" symlink, which honestly is probably a good design decision. See: 130e41ae10c305fa83df6d3158e3153b188b74c8 That does invalidate some tests, but it just means we need to update the "num_expect" in that string. I'll make the change and push it up. |
|
@dmjoy The tests are passing. Note that the original run had failures to resolve the IPFS url, but rerunning the tests passed. This is part of the tradeoff when using a distributed system, it can be a bit slow to warm up. What probably happened is we hit the IPFS gateway with the URL, it didn't have the data cached, so it started searching for the content, but timed and returned an error. But it kept trying to find that content behind the scenes so when we tried again, it found it and everything worked. I was hoping it would be a bit more seamless, but it looks like there are still some rough edges. |
|
Roger that, thanks for pushing up fixes. Any objections to merging this as-is now? |
|
Let's merge. We might need to add in a longer timeout, but let's see what it looks like for other PRs. |
Temporary workaround until this HELM PR lands: stanford-crfm/helm#3916
May consider using similar approach for other MAGNET/HELM plugins.