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 pyfunc bugs for release #188

Merged

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Mar 25, 2022

Context

This PR aims to include a couple of minor bug fixes and refactoring before the upcoming minor release of Turing:

  • Renaming of the features keyword argument of the abstract ensemble method in EnsemblerBase of the SDK to input for less ambiguity when implementing Pyfunc ensemblers
  • Transformation of the list of route responses in real-time ensembler requests to a dict of route responses indexed by route names in the SDK PyFunc interface, so as to improve usability and consistency of the predictions keyword argument of the aforementioned ensemble method
  • Addition of a name search query to the Turing API endpoint for listing registered ensemblers, so as to resolve the non-functioning ensembler search bar in the Turing UI
  • Fixing of e2e batch prediction tests due to the non-deterministic behaviour of the experiment engine
  • (Temporary) downgrading of support for the Turing SDK, ensembler job engine and ensembler service engine to only Python 3.7.*. This is to align Turing with Merlin's support for Python of that particular minor version.

@deadlycoconuts deadlycoconuts requested a review from a team March 25, 2022 05:43
@deadlycoconuts deadlycoconuts self-assigned this Mar 25, 2022
sdk/turing/version.py Outdated Show resolved Hide resolved
@deadlycoconuts deadlycoconuts marked this pull request as ready for review March 25, 2022 08:39
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Left some small comments. LGTM, pending the discussion about the e2e test case. Thank you!

api/turing/service/ensembler_service.go Outdated Show resolved Hide resolved
sdk/turing/version.py Outdated Show resolved Hide resolved
api/api/specs/ensemblers.yaml Show resolved Hide resolved
@deadlycoconuts
Copy link
Contributor Author

@romanwozniak https://github.com/gojek/turing/runs/5715592714?check_suite_focus=true#step:18:78 🥲 I'll go look at it again and see what went wrong.

@romanwozniak
Copy link
Contributor

@romanwozniak https://github.com/gojek/turing/runs/5715592714?check_suite_focus=true#step:18:78 🥲 I'll go look at it again and see what went wrong.

I have only two assumptions:

  • "hash/fnv" hashing is non-deterministic (I doubt that)
  • There is something wrong with the /batch_predict endpoint – i.e. response from one prediction request in the batch is being assigned to the wrong request before marshaling
    This is the raw response, so the issue has nothing to do with the JSON comparison
01_create_router_test.go:84: Response Payload:
        [{"code":200,"data":{"request":{"client":{"id":4}},"response":{"route_responses":[{"route":"control","data":{"version":"control"},"is_default":true}],"experiment":{"configuration":{"foo":"bar"}}}}},{"code":200,"data":{"request":{"client":{"id":7}},"response":{"route_responses":[{"route":"control","data":{"version":"control"},"is_default":true}],"experiment":{"configuration":{"foo":"bar"}}}}}]

You probably can merge this MR and keep the fix to the E2E test out of scope for now

@romanwozniak
Copy link
Contributor

@deadlycoconuts actually, it's neither nor. I can reproduce the issue by running the unit test ( which I added recently) multiple times. However, the hashing is consistent. It could be related to the floating-point precision or something similar. Let me try to figure it out

@deadlycoconuts
Copy link
Contributor Author

Thanks a lot for your help for the time being @romanwozniak !

@romanwozniak
Copy link
Contributor

romanwozniak commented Mar 28, 2022

Thanks a lot for your help for the time being @romanwozniak !

TIL: range operation over map is not deterministic: https://go.dev/blog/maps

@romanwozniak
Copy link
Contributor

Thanks a lot for your help for the time being @romanwozniak !

@deadlycoconuts It's fixed now. Sorry, for hijacking your PR. The diff is here:
https://github.com/gojek/turing/pull/188/files/481c6933929dbea18854ad9ebab07a1b3ba6e612..808818c4285983ef243ab6612545b4fff91e51a0

@deadlycoconuts
Copy link
Contributor Author

Haha no worries, thanks a lot for digging into and solving the mystery of this really elusive bug! 😂

@deadlycoconuts
Copy link
Contributor Author

deadlycoconuts commented Mar 28, 2022

Okay I'll be merging this if there aren't anything outstanding left! Thanks a lot everyone for your input! 🚀

@deadlycoconuts deadlycoconuts merged commit ca6fcba into caraml-dev:main Mar 28, 2022
@deadlycoconuts deadlycoconuts deleted the fix_pyfunc_bugs_for_release branch August 5, 2022 08:31
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.

3 participants