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 the code that aligns returned data with the output schema #335

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

karlhigley
Copy link
Contributor

@karlhigley karlhigley commented Apr 24, 2023

  • match_representations was implemented a bit weirdly and seems to have had corner cases we weren't tracking. This updates the implementation to be less clever and more straightforward, which also makes it work better.

@karlhigley karlhigley self-assigned this Apr 24, 2023
@karlhigley karlhigley added the bug Something isn't working label Apr 24, 2023
@karlhigley karlhigley added this to the Merlin 23.05 milestone Apr 24, 2023
@github-actions
Copy link

Documentation preview

https://nvidia-merlin.github.io/systems/review/pr-335

@karlhigley karlhigley changed the title Fix _to_values_offsets handle converting 1d values Fix the code that aligns returned data with the output schema Apr 24, 2023
@karlhigley karlhigley marked this pull request as ready for review April 24, 2023 20:30
@oliverholworthy
Copy link
Member

match_representations was implemented a bit weirdly and seems to have had corner cases we weren't tracking. This updates the implementation to be less clever and more straightforward, which also makes it work better.

By 'work better', is this about filtering out columns that are not present in the schema from the data passed to match representations?

@karlhigley
Copy link
Contributor Author

karlhigley commented Apr 25, 2023

By 'work better', is this about filtering out columns that are not present in the schema from the data passed to match representations?

It wasn't intended to do that, but I suppose it may have that effect. TBH, the first implementation was a bit clever and hard to think about, so instead of trying to fix it we just started over from scratch and wrote it in a fashion that was easier to understand. Once we did that, tests started passing so we called it a win. (In particular, the test for Radek's TF transformer next-item prediction example notebook.)

@karlhigley karlhigley merged commit fb7c602 into NVIDIA-Merlin:main Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] InferenceServerException: [StatusCode.INTERNAL] tuple index out of range
3 participants