- 
                Notifications
    
You must be signed in to change notification settings  - Fork 53
 
Wrap Features in Class for compatibility with ItemRetrievalScorer #574
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
Wrap Features in Class for compatibility with ItemRetrievalScorer #574
Conversation
          Click to view CI ResultsGitHub pull request #574 of commit 93aceb1a0616db222cf526acacf50dba5e8ccd80, no merge conflicts.
Running as SYSTEM
Setting status of 93aceb1a0616db222cf526acacf50dba5e8ccd80 to PENDING with url https://10.20.13.93:8080/job/merlin_models/695/console and message: 'Pending'
Using context: Jenkins
Building on master in workspace /var/jenkins_home/workspace/merlin_models
using credential nvidia-merlin-bot
 > git rev-parse --is-inside-work-tree # timeout=10
Fetching changes from the remote Git repository
 > git config remote.origin.url https://github.com/NVIDIA-Merlin/models/ # timeout=10
Fetching upstream changes from https://github.com/NVIDIA-Merlin/models/
 > git --version # timeout=10
using GIT_ASKPASS to set credentials This is the bot credentials for our CI/CD
 > git fetch --tags --force --progress -- https://github.com/NVIDIA-Merlin/models/ +refs/pull/574/*:refs/remotes/origin/pr/574/* # timeout=10
 > git rev-parse 93aceb1a0616db222cf526acacf50dba5e8ccd80^{commit} # timeout=10
Checking out Revision 93aceb1a0616db222cf526acacf50dba5e8ccd80 (detached)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f 93aceb1a0616db222cf526acacf50dba5e8ccd80 # timeout=10
Commit message: "Wrap features in class"
 > git rev-list --no-walk 1df1e026e2265ec9a75b14c030f076604ac6ca6e # timeout=10
[merlin_models] $ /bin/bash /tmp/jenkins3608752064310319777.sh
Looking in indexes: https://pypi.org/simple, https://pypi.ngc.nvidia.com
Requirement already satisfied: testbook in /usr/local/lib/python3.8/dist-packages (0.4.2)
Requirement already satisfied: nbformat>=5.0.4 in /usr/local/lib/python3.8/dist-packages (from testbook) (5.4.0)
Requirement already satisfied: nbclient>=0.4.0 in /usr/local/lib/python3.8/dist-packages (from testbook) (0.6.5)
Requirement already satisfied: traitlets>=5.1 in /usr/local/lib/python3.8/dist-packages (from nbformat>=5.0.4->testbook) (5.3.0)
Requirement already satisfied: jsonschema>=2.6 in /usr/local/lib/python3.8/dist-packages (from nbformat>=5.0.4->testbook) (4.6.1)
Requirement already satisfied: jupyter-core in /usr/local/lib/python3.8/dist-packages (from nbformat>=5.0.4->testbook) (4.10.0)
Requirement already satisfied: fastjsonschema in /usr/local/lib/python3.8/dist-packages (from nbformat>=5.0.4->testbook) (2.15.3)
Requirement already satisfied: jupyter-client>=6.1.5 in /usr/local/lib/python3.8/dist-packages (from nbclient>=0.4.0->testbook) (7.3.4)
Requirement already satisfied: nest-asyncio in /usr/local/lib/python3.8/dist-packages (from nbclient>=0.4.0->testbook) (1.5.5)
Requirement already satisfied: attrs>=17.4.0 in /usr/local/lib/python3.8/dist-packages (from jsonschema>=2.6->nbformat>=5.0.4->testbook) (21.4.0)
Requirement already satisfied: importlib-resources>=1.4.0; python_version < "3.9" in /usr/local/lib/python3.8/dist-packages (from jsonschema>=2.6->nbformat>=5.0.4->testbook) (5.8.0)
Requirement already satisfied: pyrsistent!=0.17.0,!=0.17.1,!=0.17.2,>=0.14.0 in /usr/local/lib/python3.8/dist-packages (from jsonschema>=2.6->nbformat>=5.0.4->testbook) (0.18.1)
Requirement already satisfied: entrypoints in /usr/local/lib/python3.8/dist-packages (from jupyter-client>=6.1.5->nbclient>=0.4.0->testbook) (0.4)
Requirement already satisfied: python-dateutil>=2.8.2 in /usr/local/lib/python3.8/dist-packages (from jupyter-client>=6.1.5->nbclient>=0.4.0->testbook) (2.8.2)
Requirement already satisfied: pyzmq>=23.0 in /usr/local/lib/python3.8/dist-packages (from jupyter-client>=6.1.5->nbclient>=0.4.0->testbook) (23.2.0)
Requirement already satisfied: tornado>=6.0 in /usr/local/lib/python3.8/dist-packages (from jupyter-client>=6.1.5->nbclient>=0.4.0->testbook) (6.1)
Requirement already satisfied: zipp>=3.1.0; python_version < "3.10" in /usr/local/lib/python3.8/dist-packages (from importlib-resources>=1.4.0; python_version < "3.9"->jsonschema>=2.6->nbformat>=5.0.4->testbook) (3.8.0)
Requirement already satisfied: six>=1.5 in /var/jenkins_home/.local/lib/python3.8/site-packages (from python-dateutil>=2.8.2->jupyter-client>=6.1.5->nbclient>=0.4.0->testbook) (1.15.0)
============================= test session starts ==============================
platform linux -- Python 3.8.10, pytest-7.1.2, pluggy-1.0.0
rootdir: /var/jenkins_home/workspace/merlin_models/models, configfile: pyproject.toml
plugins: anyio-3.6.1, xdist-2.5.0, forked-1.4.0, cov-3.0.0
collected 475 items / 2 skipped
 | 
    
          Documentation preview | 
    
| 
           We have tracked down the underlying reason for this in the way the save model spec is computed. And have adopted a different solution in #582  | 
    
Follow up to #554
Implementation Details 🚧
In #554 ,among other things, we changed the way features are stored and passed around to layers that need the context of the inputs. e.g. the
ItemRetrievalScorer.After #554 tests using the
RetrievalModel.evaluatemethod started breaking with tensorflow version 2.8.0. (The tests pass with the newer release 2.9.1.)The error message is:
The issue is that the model object. In this case the item_block of a two tower model thinks it accepts 2 arguments when actually it only wants one.
We can check this with the helper function
model_call_inputsand a breakpoint in theRetrievalModel.evaluatemethod. which reports that the item_block has 2 arguments. (The error "2 positional arguments but 3 were given" is not 1 vs 2 because of the additionalselfargument.)By wrapping the features in a class, somehow this results in the model helper correctly returning 1 instead of 2.
This is not necessary in tensorflow 2.9.0 and 2.9.1.
Testing Details 🔍
Tested on 2.8.0 and 2.9.0 locally
PR #573 extends our testing matrix to check both these versions