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

Enhancement - Improve playground error message and introduce new CLI command for cloudwatch logs #984

Closed
wants to merge 32 commits into from

Conversation

aybruhm
Copy link
Member

@aybruhm aybruhm commented Dec 1, 2023

Description

This PR accomplishes the following:

Work Done

  • adds a new cli command for fetching variant logs from cloudwatch
  • enhances error messages in playground for different ff (feature flags)

Related Issue

Closes #885

@aybruhm aybruhm requested a review from mmabrouk December 1, 2023 18:46
@aybruhm
Copy link
Member Author

aybruhm commented Dec 1, 2023

@bekossy, could you please check out this branch and help me run prettier on your PC to see if it prettifies the code? I'm not sure why it keeps failing, even after I've run the prettier command. If it prettifies the code, please commit and push. Thank you!

@bekossy
Copy link
Member

bekossy commented Dec 1, 2023

It's all good now @aybruhm

@aybruhm
Copy link
Member Author

aybruhm commented Dec 1, 2023

It's all good now @aybruhm

Thank you very much. What command did you run?

@bekossy
Copy link
Member

bekossy commented Dec 1, 2023

It's all good now @aybruhm

Thank you very much. What command did you run?

npm run format-fix

@aybruhm
Copy link
Member Author

aybruhm commented Dec 1, 2023

It's all good now @aybruhm

Thank you very much. What command did you run?

npm run format-fix

Strange that it didn't work for me.

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @aybruhm !
I think it makes sense to take create another error message for the cloud / ee version where these instructions don't make sense.

@aybruhm aybruhm marked this pull request as draft December 7, 2023 15:30
@aybruhm aybruhm requested a review from aakrem December 8, 2023 19:00
@aybruhm aybruhm marked this pull request as ready for review December 8, 2023 19:02
@aybruhm aybruhm requested a review from aakrem December 8, 2023 19:18
@aybruhm aybruhm requested a review from aakrem December 11, 2023 10:33
agenta-cli/agenta/cli/variant_logs.py Outdated Show resolved Hide resolved
agenta-cli/agenta/cli/variant_logs.py Outdated Show resolved Hide resolved
@aybruhm aybruhm requested a review from aakrem December 11, 2023 20:10
@aybruhm aybruhm changed the title Update - improve playground error message Enhancement - Improve playground error message and introduce new CLI command for cloudwatch logs Dec 12, 2023
Copy link
Collaborator

@aakrem aakrem left a comment

Choose a reason for hiding this comment

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

I tested this feature.
I have two concerns

  1. It looks we need an api key when using oss version:
(agenta-py3.11) ➜  baby_name_generator git:(gh/issue-885-resolve) ✗ agenta get logs --variant 6579a2322a67587e518a690c 
A new release of agenta is available: 0.5.4 → 0.6.2
To upgrade, run: pip install --upgrade agenta

Checking config file...
Retrieving variant logs...
API Key is not specified
  1. didn't we agree to use variant name instead of variant id ?
    how users are supposed to find the variant id ?

@aybruhm
Copy link
Member Author

aybruhm commented Dec 13, 2023

I tested this feature. I have two concerns

  1. It looks we need an api key when using oss version:
(agenta-py3.11) ➜  baby_name_generator git:(gh/issue-885-resolve) ✗ agenta get logs --variant 6579a2322a67587e518a690c 
A new release of agenta is available: 0.5.4 → 0.6.2
To upgrade, run: pip install --upgrade agenta

Checking config file...
Retrieving variant logs...
API Key is not specified
  1. didn't we agree to use variant name instead of variant id ?
    how users are supposed to find the variant id ?
  1. The feature is not for OSS. It's only for cloud.
  2. In the config.toml file, if there used the CLI to create a llm app and also in the playground (see image). A user can create 5 LLM apps using the template, and each variant's name will default to app.default. When this occurs, and all 5 variants were unable to start, how do we resolve getting logs for each of them using their name?
    image

@aakrem
Copy link
Collaborator

aakrem commented Dec 13, 2023

@aybruhm

  1. variant names are unique per app.
  2. it's less likely that a user will create from cli multiple agenta apps for 1 single codebase app
  3. I don't think it's a good idea at this stage we to ask users to interact with the config.toml
  4. What we can do to simply improve this, is to pass the variant_name similarly like how we create a variant name, as we create the variant name based on the file name.

Something like agenta get logs --file_name app_4.py

Or way better than this is to ask the user to choose which variants' logs are requests (I recommend this option)

agenta get logs
# we list variants
# user select variant
# we fetch logs 

@mmabrouk
Copy link
Member

@aybruhm what is the status of this one?
I think we agreed last time, that we would like to be able to see the logs in cli and UI.

@aybruhm
Copy link
Member Author

aybruhm commented Dec 20, 2023

@aybruhm what is the status of this one? I think we agreed last time, that we would like to be able to see the logs in cli and UI.

Yes, I put a hold on it because of the evaluation task. If @devgenix is less busy, I can have a quick meeting to discuss what's remaining or work on it later today.

@devgenix
Copy link
Contributor

@aybruhm what is the status of this one? I think we agreed last time, that we would like to be able to see the logs in cli and UI.

Yes, I put a hold on it because of the evaluation task. If @devgenix is less busy, I can have a quick meeting to discuss what's remaining or work on it later today.

@aybruhm sure, let's have that discussion.

@aybruhm
Copy link
Member Author

aybruhm commented Mar 5, 2024

Closing PR due to duplicate.

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.

[Bug] Errors when serving a non-functional variant due to null deployment
6 participants