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-50] Add cli.py #182

Conversation

alexkuzmik
Copy link
Collaborator

@alexkuzmik alexkuzmik commented Sep 4, 2024

Details

Adds cli.py to support commands
opik server install
opik server upgrade

In case there are any extra arguments passed, opik cli ignores them and just passes to opik-installer.
If opik-installer is not installed, it's being installed when any of these commands is called.

Testing

Tested manually, opik server install should trigger corresponding opik-server install command from opik-installer package. Similarly for opik server upgrade.

@alexkuzmik alexkuzmik requested a review from a team as a code owner September 4, 2024 15:08
Nimrod007
Nimrod007 previously approved these changes Sep 5, 2024
@Nimrod007
Copy link
Collaborator

@AndreiCautisanu please test

@CRThaze
Copy link
Contributor

CRThaze commented Sep 5, 2024

At @alexkuzmik's request, I went ahead and made some changes to this PR to integrate the opik-installer package to the SDK (as a dependency) in such a way as to acheive the following goals:

  • Keep a single parent click.Context (instead of subprocessing or invoking a second top-level instance of click)
  • Prevent the heavy ansible dependencies from being installed by pip until invoking a command that requires it.

To accomplish this I:

  • Added the dynamic installation of the heavy dependencies to opik-installer as a Click decorator.
  • Ensured that we can still guarantee a minimum version of the ansible-playbill (and by proxy ansible) packages
  • Adjusted the debug_set function to work properly when opik-server is not the root context, by traversing the each context generation.
  • Changed how the Opik SDK's cli.py adds the server sub-group so that it simply uses the imported group from opik-installer
  • Added some logic to cli.py to ensure that both -v and -h can be called on the root command (opik -v & opik -h) in the same way as in the server subcommand.

@alexkuzmik
Copy link
Collaborator Author

alexkuzmik commented Sep 5, 2024

LGTM

@AndreiCautisanu
Copy link
Contributor

Tested on Mac: issue with Docker Daemon start after starting Docker Desktop - cannot confirm whether this is fault of installer or not fully clean machine after uninstalling Docker Desktop to test. Summary: after uninstalling Docker and running installer, the Docker Desktop instance that is started with the Open Docker Desktop task throws an error or does not start at all, causing the Wait for Docker daemon to start task to fail due to a timeout. If Docker Desktop is already installed on the system or if it's restarted during installation - everything works fine. Other than this, all ok

Tested on Linux: all ok

@japdubengsub japdubengsub self-requested a review September 6, 2024 15:44
@alexkuzmik alexkuzmik merged commit bded09e into main Sep 8, 2024
6 checks passed
@alexkuzmik alexkuzmik deleted the OPIK-50-add-commands-for-installing-start-and-stop-the-opik-server branch September 8, 2024 12:18
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.

5 participants