-
Notifications
You must be signed in to change notification settings - Fork 46
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
lamda function for context summarizer #301
Conversation
@saminegash is this a new file? why aren't we just modify the existing one? |
can you be more descriptive about what this pr fixes? |
This is kind of modifying the lambda function and this is to make the code uptodatw |
|
||
|
||
|
||
ACCESS_KEY = os.environ['ACCESS_KEY'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use import sherpa_ai.config as cfg
and then access the variables via cfg.*
, instead of referencing them directly from os.environ. See other files like vectorstores.py for example usage.
|
||
|
||
|
||
# response = s3_client.get_object(Bucket=source_bucket, Key=source_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these lines, since they're all commented out anyway
# Read the file content from the source bucket | ||
response = s3_client.get_object(Bucket=source_bucket, Key=source_key) | ||
print("Response :",response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print() -> logger.debug() everywhere
addressed and re requesting review
|
||
human_template = """Consolidate the multiple parts of the text into one \ | ||
human_template = """Consolidate the multiple parts of the text into one \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to:
human_template = """Consolidate the multiple parts of the text into one \ coherent essay or article that accurately captures the content of the multiple\ parts without losing any information. The entire text is delimited in triple backticks and the parts are divided by #heading:\n ```{essays}```"""
|
||
# takes about 2-3 minutes to run | ||
# summary= full_transcript2essay(raw_transcript) | ||
summary = full_transcript2essay(first_part) # considering only talk part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename all instances of this variable to talk_summarized and add the content of this variable to the output rst file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I can see the comment but I don't understand why you require this change and how it can be added.
the Essay is derived from this summary variable
Your checklist for this pull request
Thank you for submitting a pull request! To speed up the review process, please follow this checklist:
make format
)/docs
)pytest tests
(offline mode)Additional steps for code with networking dependencies:
pytest tests --external_api
(online mode, making network calls)Description
Describe your pull request here.
What does this PR implement or fix? Explain.
If this PR resolves any currently open issues then mention them like this:
Closes #xxx
.Github will close such issues automatically when your PR is merged into
main
.Any relevant logs, error output, etc?
Any other comments? For example, will other contributors need to install new libraries via
poetry install
after picking up these changes?💔Thank you!