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

fix(Monitoring): Serializable log middleware #1908

Merged
merged 18 commits into from Nov 22, 2022

Conversation

frascuchon
Copy link
Member

Also, review and format the class name

@frascuchon frascuchon force-pushed the bugfixes/serializable-asgi-middleware branch from 16a12d2 to a556026 Compare November 15, 2022 20:58
@@ -67,16 +67,52 @@ def token_classification_mapper(inputs, outputs):


def text_classification_mapper(inputs, outputs):

Choose a reason for hiding this comment

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

@frascuchon I updated the logic for the default mapper

src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
src/argilla/monitoring/asgi.py Show resolved Hide resolved
]

if records:
records = self._records_mapper(inputs, outputs)

Choose a reason for hiding this comment

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

maybe it makes sense to create a try-except for the records mapper that provides some information about the input-output and that the _records_mapper is causing the error and can be adapted manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
src/argilla/monitoring/asgi.py Show resolved Hide resolved
src/argilla/monitoring/asgi.py Show resolved Hide resolved
src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
src/argilla/monitoring/asgi.py Show resolved Hide resolved
]

if records:
records = self._records_mapper(inputs, outputs)
Copy link
Member Author

Choose a reason for hiding this comment

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

+1

src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
src/argilla/monitoring/asgi.py Outdated Show resolved Hide resolved
@frascuchon frascuchon force-pushed the bugfixes/serializable-asgi-middleware branch from b924651 to c533ef6 Compare November 17, 2022 16:58
@frascuchon frascuchon changed the base branch from main to develop November 17, 2022 16:59
@frascuchon
Copy link
Member Author

Done @davidberenstein1957

Now, the middleware is using a BaseMonitor object, so the logic to send data to Argilla remains aligned with the rg.monitor behavior.

The code has changed a little bit. The main thing here is to provide the records properly in the _prepare_argilla_data method (old log_to_argilla).

Let me know if you have questions.

@frascuchon frascuchon self-assigned this Nov 18, 2022
@frascuchon frascuchon added this to the v1.1.0 milestone Nov 21, 2022
@frascuchon
Copy link
Member Author

Cool @davidberenstein1957 !!! Great work.

Only a comment. It would be nice if we add a minimal test for the GET method part.

question: Do we need to change the record_mapper for the rest of the tasks?

@davidberenstein1957
Copy link
Member

@frascuchon

I set the record_mapper to be a required variable to make the usage a bit more explicit and avoid potential questions. So, people always need to define a record mapper, especially since using it with ASGI is going to be usecase dependent. For the tests and as examples, I left the current record_mappers within the asgi file.

I will add a GET test.

@davidberenstein1957 davidberenstein1957 marked this pull request as ready for review November 22, 2022 15:50
@davidberenstein1957 davidberenstein1957 merged commit 53a57f7 into develop Nov 22, 2022
@davidberenstein1957 davidberenstein1957 deleted the bugfixes/serializable-asgi-middleware branch November 22, 2022 15:51
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