-
Notifications
You must be signed in to change notification settings - Fork 67
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(weave): Move weave python under sdk #2648
Conversation
199ccd9
to
540ea30
Compare
84ad42c
to
944e79d
Compare
sdks/python/weave/clear_cache.py
Outdated
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.
This move is going to break local builds again
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.
I think it breaks helm, yeah good point.
Gonna keep clear_cache.py
where it is and leave a note.
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.
I think you might want to modify the docker file to actually load from weave_query dir, rather than this file
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.
We already do:
https://github.com/wandb/core/blame/master/onprem/local/Dockerfile#L46
But I don't think that would work for helm:
https://github.com/wandb/helm-charts/blob/main/charts/operator-wandb/charts/weave/templates/deployment.yaml#L93
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.
hmm... ok. no more changes needed here... but FWIW, my understanding is that:
- https://github.com/wandb/core/blame/master/onprem/local/Dockerfile#L46 -> this docker file puts the file at the correct
weave/clear_cache.py
dir - https://github.com/wandb/helm-charts/blob/main/charts/operator-wandb/charts/weave/templates/deployment.yaml#L93 picks that up.
Implied by your comment is that https://github.com/wandb/helm-charts/blob/main/charts/operator-wandb/charts/weave/templates/deployment.yaml#L93 relies on clear_cache.py
being in this repository at this location, but i am not sure that is true.
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.
this is not actually a delete. It was moved to sdks/python
and then symlinked back here
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.
this is not actually a delete. It was moved to sdks/python
and then symlinked back here
6d13741
to
299e3b9
Compare
sdks/python
in prep for other langs TheREADME.md
andLICENSE
are symlinked fromsdks/python
to the repo root for nowweave/clear_cache.py
for compat