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

Logger sort of fixed #22

Closed
wants to merge 7 commits into from
Closed

Logger sort of fixed #22

wants to merge 7 commits into from

Conversation

canyon289
Copy link
Collaborator

llmv_routes is not using the configured logger, detailed in the issue below. Still trying to figure it out

#21

llmv_routes.py Outdated
@@ -39,6 +41,12 @@ def llm_verification_index():
@authed_only
def generate_for_challenge():
"""Add a route to CTFd for generating text from a prompt."""
log.error("Log name %s, %s", log.name, log.level)
log.info("Received test generation request but his logger is not working for some reason")
log.error("NEW Received test generation request. Testing error level logging too")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason info level logs don't print to console which is the main issue
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something funny is going on where the llmv_routes logging is going to the wrong logger

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@canyon289 canyon289 marked this pull request as ready for review September 23, 2023 17:44
@@ -11,13 +11,12 @@
from .llmv_models import LlmSubmissionChallenge, fill_models_table
from .llmv_routes import add_routes

log = getLogger(__name__)
log = initialize_llmvctfd_loggers(__name__)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes an issue where other modules were not logging to console

# Load the Vanilla Neox API key from the config file.

raw_response = post(url=route,
headers={'Authorization': f'Bearer {token}'},
json={'prompt': prompt, "preprompt" : preprompt, "model": model})

try:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Provides a much better exception message than the json decode I was getting before with the bad llmv_router

@canyon289 canyon289 changed the title Logger [not] fixed Logger sort of fixed Sep 23, 2023
@canyon289
Copy link
Collaborator Author

Ready for review. PR doesn't smoothly fix logging issue, just changes everything to error so it logs to console. It does add an extra exception handler which which provides readable exception of what is going wrong where

I propose merging and then fixing logging handler issues in a separate PR.

@comath
Copy link
Contributor

comath commented Sep 24, 2023

The logger seemed to work fine on my machine 😁. I think the solution to this is a new function in llm_logger that returns the appropriate logger, guaranteed.

I'm not a fan of logging to error, but the rest looks good

@canyon289
Copy link
Collaborator Author

The logger seemed to work fine on my machine 😁. I think the solution to this is a new function in llm_logger that returns the appropriate logger, guaranteed.

I'm not a fan of logging to error, but the rest looks good

Tried this but it seems to make things worse. Leaving this branch open for now, moved the json exception handling to another PR since thats the bigger problem anyway

#22

=

@canyon289 canyon289 closed this Sep 24, 2023
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.

None yet

2 participants