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

Add Transformers4Rec repo test #1049

Merged
merged 9 commits into from
Apr 18, 2023
Merged

Conversation

edknv
Copy link
Contributor

@edknv edknv commented Apr 4, 2023

This PR adds Github actions that run the unit tests in transformers4rec. The motivation for adding these tests is to detect if changes in Dataloader could potentially break T4Rec since T4Rec now depends on Models. These downstream repo tests are similar to how they are set up in Core.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Documentation preview

https://nvidia-merlin.github.io/models/review/pr-1049

@edknv edknv self-assigned this Apr 5, 2023
@edknv edknv added enhancement New feature or request ci labels Apr 5, 2023
@edknv edknv added this to the Merlin 23.04 milestone Apr 5, 2023
@rnyak rnyak requested a review from nv-alaiacano April 5, 2023 13:06
tox.ini Outdated
passenv=GIT_COMMIT
allowlist_externals = git
commands =
; the GIT_COMMIT env is the current commit of the core repo
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be the current commit of models repo.

branch=main
if [[ $ref_type == "tag"* ]]
then
git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/heads/release*:refs/remotes/origin/release*
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you leave a comment about what is happening here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is just standard boilerplate for fetching a shallow copy of minimal size

Copy link
Member

Choose a reason for hiding this comment

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

This can be replaced with something like this now. Moved this into a reusable action in our .github repo.

      - name: Get Branch name
        id: get-branch-name
        uses: NVIDIA-Merlin/.github/actions/branch-name@main
      - name: Run tests
        run: |
          branch="${{ steps.get-branch-name.outputs.branch }}"
          GIT_COMMIT=`git rev-parse HEAD` tox -e tox -e py38-transformers4rec-cpu -- $branch

Note that this doesn't currently work with our self-hosted runners because the action is implemented with docker and we can't run docker in our self-hosted runners

@edknv edknv marked this pull request as ready for review April 6, 2023 16:47
@edknv edknv merged commit 78f2732 into NVIDIA-Merlin:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants