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

implement review suggestions #1062

Merged
merged 3 commits into from
Apr 26, 2023
Merged

implement review suggestions #1062

merged 3 commits into from
Apr 26, 2023

Conversation

radekosmulski
Copy link
Contributor

We merged #1045 to meet the cutoff, but there were some outstanding review comments from @rnyak and @edknv. This PR implements the suggestions mentioned on #1045

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link

Documentation preview

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

@radekosmulski
Copy link
Contributor Author

rerun tests

@rnyak rnyak added this to the Merlin 23.05 milestone Apr 19, 2023
tb.execute_cell(list(range(0, 38)))

with run_triton_server("/tmp/ensemble", grpc_port=8001):
with run_triton_server("/tmpdir/ensemble", grpc_port=8001):
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be something like f"{tmpdir}/ensemble to convert the temp directory name into a the path.

And the tmpdir fixture needs to be added as a named in the arguments list to be available inside the body of the test. There's a notebook test here that uses tmpdir as an example
https://github.com/NVIDIA-Merlin/models/blob/v23.02.00/tests/unit/tf/examples/test_06_advanced_own_architecture.py#L13

@radekosmulski
Copy link
Contributor Author

rerun tests

@oliverholworthy
Copy link
Member

rerun tests

adding comments to the PR to trigger re-runs of tests I don't think is required/works anymore. This was previously to re-trigger a test run of the GPU-based tests that were running in Jenkins. Now that we have a self-hosted GitHub Actions runner for that, the way to trigger a re-run is to open the Details page of the relevant failed check and use the Re-run jobs button at the top right of the page.

image

@radekosmulski
Copy link
Contributor Author

Thank you for the heads up, @oliverholworthy! 🙂

@radekosmulski radekosmulski merged commit c016773 into main Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants