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

docstring fix: Example of elasticsearch custom query that works correctly with substitions. Added unit test. Added verbose kwarg for debugging to finder and retriever. #359

Closed
wants to merge 9 commits into from

Conversation

skirdey
Copy link
Contributor

@skirdey skirdey commented Sep 7, 2020

No description provided.

@skirdey skirdey changed the title Adding verbose param to to finder and retriever to help with debugging. docstring fix: Example of elasticsearch custom query that works correctly with substitions. Added unit test. Sep 7, 2020
@skirdey skirdey changed the title docstring fix: Example of elasticsearch custom query that works correctly with substitions. Added unit test. docstring fix: Example of elasticsearch custom query that works correctly with substitions. Added unit test. Added verbose kwarg for debugging to finder and retriever. Sep 7, 2020
@tholor
Copy link
Member

tholor commented Sep 10, 2020

Hey @skirdey,

Thanks for working on the "verbose" option. Let us know when you are done and we should review.

@tholor
Copy link
Member

tholor commented Sep 10, 2020

Maybe one comment already from our side: We believe it's useful to log more debug messages and the ones that you added seem helpful. However, we believe it would be nicer to use logger.debug("something") instead of passing a verbose arg. The user can then configure the logging level via logging.getLogger('haystack').setLevel(logging.DEBUG). This would also align nicely with some of our logging solutions for production setups.

@skirdey
Copy link
Contributor Author

skirdey commented Sep 10, 2020

Hi tholor,
I wanted to use debug, but I also wanted to be able to see what is going on in production when I pass verbose=True to api call and seeing only relevant information in production logs. Let me know what do you think about it.

@tholor
Copy link
Member

tholor commented Sep 14, 2020

Ok, I totally see the use case of debugging API requests in a production setup. What I would like to understand better is why you don't do this by setting the global logging level to debug and search in the logs for the request of interest?
We have a similar setup with our deployments where we log an additional request_id that helps to find the problematic request. Is it because it's difficult for you to switch log levels in production?

If we go forward with the verbose arg, I see some potential downsides:

  • We'll end up having that arg for pretty much all methods in the framework
  • We'll have rather ugly code blocks all around the place that either log due to verbose = True or to the log level being debug

Therefore, I would only like to include it in Haystack if there's a big advantage for debugging that outweighs the above points.
Happy to hear your thoughts on that!

@skirdey
Copy link
Contributor Author

skirdey commented Sep 14, 2020

Good point, let me try setting loglevel for haystack and see what is the output in prod. I'll adjust the PR accordingly.

@skirdey skirdey closed this Sep 16, 2020
@tholor
Copy link
Member

tholor commented Sep 16, 2020

I suppose switching log level worked for you?
Are there any additional messages that helped you debugging in your case and would be helpful to add to the framework? We don't have that many yet...

@skirdey
Copy link
Contributor Author

skirdey commented Sep 16, 2020

I will need to do another PR for the debug messages, it does work.

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