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

Restoring tox.ini and .github/workflows/tensorflow.yml #1023

Merged
merged 3 commits into from
Mar 22, 2023

Conversation

gabrielspmoreira
Copy link
Member

Goals ⚽

Restoring tox.ini and .github/workflows/tensorflow.yml to reverse changes done to run models CI on a specific dataloader branch

@jperez999 jperez999 added ci chore Maintenance for the repository labels Mar 17, 2023
@jperez999 jperez999 added this to the Merlin 23.03 milestone Mar 17, 2023
@github-actions
Copy link

Documentation preview

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

# Enforcing the PR that implements the dataloader features changes
#python -m pip install --upgrade git+https://github.com/NVIDIA-Merlin/dataloader.git@{posargs:main}
python -m pip install --upgrade git+https://github.com/bschifferer/dataloader.git@change_output
python -m pip install --upgrade git+https://github.com/NVIDIA-Merlin/dataloader.git@{posargs:main}
Copy link
Member

Choose a reason for hiding this comment

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

We might need to add --no-deps to each of these pip installs of merlin packages to avoid the subsequent pip installs changing the version of a previous merlin package

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @oliverholworthy . I agree.
My concern is if we use --no-deps here it won't install none of the dependencies, including external PyPy packages. Could we assume that external packages are already installed in the container (so that we can use --no-deps)?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's safe to do --no-deps because the merlin packages installed here (e.g. the dataloader) are already part of the Merlin Models package requirements which get installed as part of every tox environment by default.

My understanding is that main reason we have these lines here is to override the version that's installed for the release branch builds.

…nges done to run CI on a specific dataloader branch
@jperez999 jperez999 merged commit f2a4980 into main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintenance for the repository ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants