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

Docs: Create your own batch processor documentation inconsistency #2688

Closed
1 task done
erikayao93 opened this issue Jul 5, 2023 · 7 comments · Fixed by #4024
Closed
1 task done

Docs: Create your own batch processor documentation inconsistency #2688

erikayao93 opened this issue Jul 5, 2023 · 7 comments · Fixed by #4024
Assignees
Labels
batch Batch processing utility documentation Improvements or additions to documentation

Comments

@erikayao93
Copy link
Contributor

What were you searching in the docs?

While investigating the capabilities of the batch processor in Python for a similar implementation in TypeScript, I found an inconsistency in the description compared to the code example.

The description states that developers should inherit the BasePartialProcessor class, which would require them to implement the abstract methods of prepare, clean, and process_record (or async_process_record).

However, the code example creates a class that inherits the BasePartialBatchProcessor class.

Is this related to an existing documentation section?

https://docs.powertools.aws.dev/lambda/python/2.19.0/utilities/batch/#create-your-own-partial-processor

How can we improve?

There should be clarity on which class developers are meant to inherit from.

If developers should inherit from BasePartialProcessor, the code example should be updated so that the code is usable. This may require other backend updates, as several pieces of functionality in the code example are reliant on capabilities of the BasePartialBatchProcessor class.

Got a suggestion in mind?

No response

Acknowledgment

  • I understand the final update might be different from my proposed suggestion, or refused.
@erikayao93 erikayao93 added documentation Improvements or additions to documentation triage Pending triage from maintainers labels Jul 5, 2023
@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Jul 7, 2023
@heitorlessa heitorlessa moved this from Triage to Working on it in Powertools for AWS Lambda (Python) Jul 7, 2023
@heitorlessa heitorlessa self-assigned this Jul 7, 2023
@heitorlessa
Copy link
Contributor

heitorlessa commented Jul 7, 2023

great catch @erikayao93 - we had a major refactor recently and the old name wasn't updated in this section of the docs. I'll double check the correct one and update the docs today.

UPDATE: Correct one: BasePartialProcessor since it's the ABC. BasePartialBatchProcessor is the new one that has the base implementation for SQS/Kinesis/DynamoDB -- the former only has the contracts.

THANK YOU!

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Jul 7, 2023
@erikayao93
Copy link
Contributor Author

One more thing to note here -- the process_partial_response method that is used in the example expects a BasePartialBatchProcessor -- not a BasePartialProcessor, so I don't think the example would be functional in this case. The example could call directly as a context manager instead?

@github-actions
Copy link
Contributor

This is now released under 2.20.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Jul 14, 2023
@github-project-automation github-project-automation bot moved this from Working on it to Coming soon in Powertools for AWS Lambda (Python) Jul 14, 2023
@leandrodamascena
Copy link
Contributor

One more thing to note here -- the process_partial_response method that is used in the example expects a BasePartialBatchProcessor -- not a BasePartialProcessor, so I don't think the example would be functional in this case. The example could call directly as a context manager instead?

Hello @erikayao93! Thank you for pointing that out. Indeed, the process_partial_response signature expects a BasePartialBatchProcessor, but we can use BasePartialProcessor and implement the process method in this processor. To avoid any problems if someone wants to create their own BatchProcessor and guide our customers in the right way, let me recreate this example with comments and a better example.

Reopening the issue.

@github-project-automation github-project-automation bot moved this from Coming soon to Pending review in Powertools for AWS Lambda (Python) Jul 19, 2023
@leandrodamascena leandrodamascena added the batch Batch processing utility label Jul 19, 2023
@leandrodamascena
Copy link
Contributor

I'm working on it and plan to create a PR tomorrow.

Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Mar 27, 2024
Copy link
Contributor

This is now released under 2.36.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Mar 27, 2024
@leandrodamascena leandrodamascena moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
batch Batch processing utility documentation Improvements or additions to documentation
Projects
Status: Shipped
3 participants