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

OPIK-814 [SDK] Implement opik healthcheck CLI command / python function to validate the current... #1341

Merged
merged 25 commits into from
Feb 27, 2025

Conversation

japdubengsub
Copy link
Contributor

@japdubengsub japdubengsub commented Feb 20, 2025

Details

Output examples:
image

image

@alexkuzmik
Copy link
Collaborator

alexkuzmik commented Feb 21, 2025

@japdubengsub some feedback to your draft :)
Let's:

  1. make dependencies printing optional. It might be useful only in a small subset of cases but it spams the output too much.
  2. move the logic from cli.py to another module(s) and only make cli.py invoke a function from this module(s).
  3. provide a proper logic separation. The presentation (printing) logic should be separated from the business one (all the different checks). For every check, we want to have a function that returns the result of the check. And after that this result is used to print an output. If later we want to have a python function that just returns you a dataclass instance with all the checks results (for example to help us with opik config in integrations) - it should be easy to implement by just calling business logic functions and saving their results. Right now it would be a problem because the checks live inside the log messages.
  4. use rich for terminal outputs, we already have it in our dependencies and even use it in forwarding_server, evaluation and maybe somewhere else. Rich will make the output way prettier and will be a nicer UX. (btw you can try asking an LLM to suggest a better rich formatting, it often performs good on such tasks :)

You probably haven't got to backend/httpx errors results parsing, so I'm not commenting this part yet

@japdubengsub japdubengsub marked this pull request as ready for review February 21, 2025 19:16
@japdubengsub japdubengsub requested a review from a team as a code owner February 21, 2025 19:16
Copy link
Collaborator

@alexkuzmik alexkuzmik left a comment

Choose a reason for hiding this comment

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

Please don't forget about the general SDK guidelines.

Also, whenever you're adding a new code, pay extra attention to the questions:

  • do we actually need an extra function for that?
  • if we need it, is there a better place for it?

@alexkuzmik
Copy link
Collaborator

Looks way better, thanks!

One last UX improvement, we currently have a message:

Error while checking backend workspace availability: [Errno 111] Connection refused

Can't connect to the backend service. If you are using local Opik deployment, please check 
https://www.comet.com/docs/opik/self-host/local_deployment

If you are using cloud version - please check your internet connection.

We know Opik URL, so it's better to:

  1. Show this URL with the workspace during/after the attempt to connect. It will help users to understand what exactly they are connecting too faster.
  2. not to print If you are using ..., If you are using, because we know exactly when the user is using cloud (by looking at URL).

After that we can merge

@japdubengsub japdubengsub merged commit d47c20d into main Feb 27, 2025
43 checks passed
@japdubengsub japdubengsub deleted the OPIK-814 branch February 27, 2025 13:53
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