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

Add guidelines for local run #983

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

liyaka
Copy link
Contributor

@liyaka liyaka commented Jan 5, 2025

Details

Issues

Resolves #

Testing

Documentation

@liyaka liyaka requested a review from amirzatcomet January 5, 2025 13:08
@liyaka liyaka requested review from a team as code owners January 5, 2025 13:08
amirzatcomet
amirzatcomet previously approved these changes Jan 5, 2025
Copy link

@amirzatcomet amirzatcomet left a comment

Choose a reason for hiding this comment

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

lgtm

Stop backend container, cause you don't need it..


3. Update your Opik Backend configuration to connect to all databases on the localhost, and run it
Copy link
Contributor

Choose a reason for hiding this comment

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

When you say localhost, do you mean the one set in step 1? If that's the case, I think it's worth mentioning

Copy link

@amirzatcomet amirzatcomet Jan 5, 2025

Choose a reason for hiding this comment

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

I didn't have to add special setup for my Opik BE, it looks like it's already set up with those default ports
Just had to change the nginx setting above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@idoberko2 i mean your local backend configuration, not in docker-compose.yaml

Choose a reason for hiding this comment

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

@liyaka like I said, I changed nothing and it worked, are you sure any local config changes are required to the BE?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Amir is right here, values in apps/opik-backend/config.yml already default to this. If not, we should fix it like I recently did here: #958

I think we can remove this bullet point number 3.

```bash
docker compose -f docker-compose.yaml -f docker-compose.override.yaml up -d
```
Stop backend container, cause you don't need it..
Copy link
Collaborator

Choose a reason for hiding this comment

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

why stop the backend? cant we just disable it in docker compose override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these instructions were without code changes (at least for now)
If you run docker-compose with the list of the components without the backend, it still runs because frontend is dependent on it
You can comment-out the depend_on for frontend service and run it as

docker compose -f docker-compose.yaml -f docker-compose.override.yaml up -d mysql redis clickhouse frontend

Copy link
Collaborator

@andrescrz andrescrz Jan 7, 2025

Choose a reason for hiding this comment

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

We can improve this in the near future by adding docker compose groups, so by default we start everything while we can choose to start: databases only, databases + BE, everything but BE etc.

In the meantime, documenting that that the BE container should be stop is good enough.

@@ -51,6 +51,31 @@ This will expose the following services to the host machine
- Backend: Available on ports 8080 and 3003


## Run Opik backend locally and the rest with docker-compose
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Run Opik backend locally and the rest with docker-compose
## Run Opik backend locally and the dependencies with docker-compose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frontend is not a dependency :)

@amirzatcomet
Copy link

This was supposed to be a quick commit for our own devs to start working, if we're going to do this for the community we have to do it right, test it, document properly and make any code changes required to support it so that it's easiest for the user to get it working without messing it up.



3. Update your Opik Backend configuration to connect to all databases on the localhost, and run it


## Stop opik

Choose a reason for hiding this comment

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

?

Copy link
Collaborator

@andrescrz andrescrz left a comment

Choose a reason for hiding this comment

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

Left some comments, but it should be good to go soon.

Please update the PR description.

```bash
docker compose -f docker-compose.yaml -f docker-compose.override.yaml up -d
```
Stop backend container, cause you don't need it..
Copy link
Collaborator

@andrescrz andrescrz Jan 7, 2025

Choose a reason for hiding this comment

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

We can improve this in the near future by adding docker compose groups, so by default we start everything while we can choose to start: databases only, databases + BE, everything but BE etc.

In the meantime, documenting that that the BE container should be stop is good enough.

Stop backend container, cause you don't need it..


3. Update your Opik Backend configuration to connect to all databases on the localhost, and run it
Copy link
Collaborator

Choose a reason for hiding this comment

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

Amir is right here, values in apps/opik-backend/config.yml already default to this. If not, we should fix it like I recently did here: #958

I think we can remove this bullet point number 3.

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