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

minor modification to move the top_k param to the environment variables for convenience. #226

Conversation

ppmarkus
Copy link
Contributor

Depending on the use case we generally require many more rows to be returned.

@MohammadrezaPourreza
Copy link
Contributor

@ppmarkus Thanks for your contribution we really appreciate all your help.
Since agent also has a tool to run a SQL query we have to also make the following change to the dataherald_sqlagent.py file:
TOP_K = os.getenv("MAX_QUERY_RETURN_ROWS", "50"))
Could you please also include this in your PR?

@ppmarkus
Copy link
Contributor Author

updated and tested before and after.
Without UPPER_LIMIT_QUERY_RETURN_ROWS env, rows limited to 50.
When set, returned rows changed up to new set limit.
Casting env var to int was necessary, otherwise got error: unsupported operand type(s) for +: 'int' and 'str' because top_k needs to be an int.

@MohammadrezaPourreza MohammadrezaPourreza merged commit efdd3b0 into Dataherald:main Oct 30, 2023
DishenWang2023 pushed a commit that referenced this pull request May 7, 2024
…es for convenience. (#226)

* minor modification to move the top_k param to the environment variables for convenience.

* renamed MAX_QUERY_RETURN_ROWS ->  UPPER_LIMIT_QUERY_RETURN_ROWS to make it easier to understand

* updated docs to match env var change

* os.getenv returns a 'str' type so it's necessary to cast to int

---------

Co-authored-by: Paul Markus <[email protected]>
DishenWang2023 added a commit that referenced this pull request May 7, 2024
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.

4 participants