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

[ENH] Made base_api_url an environment variable #37

Merged
merged 9 commits into from
Aug 14, 2024
Merged

Conversation

Raya679
Copy link
Contributor

@Raya679 Raya679 commented Aug 10, 2024

Changes proposed in this pull request:

  • Made base_api_url an environment variable.
  • Updated documentation.

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@Raya679 Raya679 requested a review from rmanaem August 10, 2024 07:06
@Raya679
Copy link
Contributor Author

Raya679 commented Aug 10, 2024

Hii @rmanaem, I have made the base_api_url an environment variable here. Please take a look and suggest any changes if required.
Thanks!

app/api/url_generator.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rmanaem rmanaem 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 @Raya679!
I left suggestions to rename the env var to essentially mirror the env var we are already using in the query tool.
🍒 I think it's a good idea to have a way to make sure the new env var is set, throw an error to inform the user in case it's not and a test for that. Since the implementation simply concatenates the env var with the output from the llm to send the request, we can end up with http errors if the env var is not set properly. Let me know what you think.

@rmanaem rmanaem added the pr-internal Non-user-facing code improvement, will increment patch version when merged (0.0.+1) label Aug 13, 2024
Raya679 and others added 5 commits August 13, 2024 21:35
Co-authored-by: Arman Jahanpour <77515879+rmanaem@users.noreply.github.com>
Co-authored-by: Arman Jahanpour <77515879+rmanaem@users.noreply.github.com>
Co-authored-by: Arman Jahanpour <77515879+rmanaem@users.noreply.github.com>
@Raya679
Copy link
Contributor Author

Raya679 commented Aug 13, 2024

Hi @rmanaem , thanks for the review!
I have made the changes suggested. Also I have removed the default value None of NB_API_QUERY_URL since now we get an error if it is not set.
Please do suggest any changes if required.

@Raya679 Raya679 requested a review from rmanaem August 13, 2024 16:50
Copy link
Contributor

@rmanaem rmanaem left a comment

Choose a reason for hiding this comment

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

🍒

app/api/url_generator.py Outdated Show resolved Hide resolved
@Raya679 Raya679 requested a review from rmanaem August 13, 2024 17:33
@Raya679
Copy link
Contributor Author

Raya679 commented Aug 13, 2024

Hii @rmanaem, I have made the changes.
Please take a look and do suggest any changes if required.

Thanks!

Copy link
Contributor

@rmanaem rmanaem left a comment

Choose a reason for hiding this comment

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

🧑‍🍳

@Raya679 Raya679 merged commit c119ea4 into main Aug 14, 2024
4 checks passed
@Raya679 Raya679 deleted the base-api-url branch August 14, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-internal Non-user-facing code improvement, will increment patch version when merged (0.0.+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make base_api_url an Environment Variable
2 participants