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

Handle the base exception from the requests library #396

Merged
merged 5 commits into from
Oct 11, 2024

Conversation

mgdaily
Copy link
Contributor

@mgdaily mgdaily commented Oct 4, 2024

We were handling one of the subclasses, and some errors slipped through, causing missed reductions.

Also update our deploy to use in-cluster URLs to speed up the DNS resolution and make us more resilient to DNS issues.

Sorry for all of the whitespace changes, but it looks like we had some extra end-of-line whitespace in the changelog.

We were handling one of the subclasses, and some errors slipped through, causing missed reductions.
Copy link

@jchate6 jchate6 left a comment

Choose a reason for hiding this comment

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

I don't really understand the reason for the banzai changes with urls, but I trust that you do.

I have an actual question about versioning, but again, I might be missing things, so I'm clicking approve.

@@ -6,20 +6,20 @@
# will be started when the CPU usage rises above the configured threshold.
horizontalPodAutoscaler:
enabled: true
minReplicas: 15
maxReplicas: 15
minReplicas: 25
Copy link

Choose a reason for hiding this comment

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

We should probably talk about how to introduce autoscaling someday when you have the bandwidth...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to set a max replicas here? Or just assume if we only set a min, they are equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably talk about how to introduce autoscaling someday when you have the bandwidth...

Jashan did prototype this at one time. https://github.com/LCOGT/banzai/pull/359/files

Autoscaling based on queue size sounds great Joey, but I am very hesitant to commit my time to this as a development priority. I think it is relatively high effort to low gain, given that we can simply run more pods than we need most of the time.

I would love if someone other than me could spearhead that effort.

Copy link
Contributor Author

@mgdaily mgdaily Oct 7, 2024

Choose a reason for hiding this comment

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

Do we need to set a max replicas here? Or just assume if we only set a min, they are equal?

Yes we need to. https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#horizontalpodautoscalerspec-v2-autoscaling

This has always been the case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. Ok. github split the file and I missed the next line.


image:
repository: ghcr.io/lcogt/banzai
tag: "1.18.1"
tag: "1.18.2"
Copy link

Choose a reason for hiding this comment

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

Should this match the version number above in the changes? That's currently 1.18.3...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

This is to speed up and make more robust the serialization of frames endpoint responses, particularly for BPMs which have many thousands of related frames.
@mgdaily mgdaily merged commit 21ac9ba into main Oct 11, 2024
6 checks passed
@mgdaily mgdaily deleted the fix/photometry-exception-handling branch October 11, 2024 20:26
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.

3 participants