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 NeMo #7

Merged
merged 29 commits into from Jan 3, 2022
Merged

Add NeMo #7

merged 29 commits into from Jan 3, 2022

Conversation

SeanNaren
Copy link
Contributor

@SeanNaren SeanNaren commented Nov 23, 2021

Before submitting

Starting with the release/CPU test

  • Was this discussed/approved via a GitHub issue? (no need for typos and docs improvements)
  • Did you create/update your configuration file?
  • Did you add your config to CI - GitHub action and Azure pipeline?
  • Are all integration tests passing?

What does this PR do? [optional]

Resolves NVIDIA/NeMo#3502

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda marked this pull request as draft November 23, 2021 17:56

testing:
dirs:
- tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to run all their tests?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why wouldn't we?

Copy link
Contributor

@tchaton tchaton Nov 25, 2021

Choose a reason for hiding this comment

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

Just an open question. I don't know how long their CI takes, but definitely like the idea.

@Borda Borda marked this pull request as ready for review November 24, 2021 16:38
@Borda
Copy link
Member

Borda commented Nov 24, 2021

@SeanNaren seems they also require several binaries to run the tests...

@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #7 (98c6daf) into main (f99cb1f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main       #7   +/-   ##
=======================================
  Coverage   85.09%   85.09%           
=======================================
  Files           2        2           
  Lines         208      208           
=======================================
  Hits          177      177           
  Misses         31       31           
Flag Coverage Δ
Linux 85.09% <ø> (ø)
Windows 85.09% <ø> (ø)
cpu 85.09% <ø> (ø)
macOS 85.09% <ø> (ø)
pytest 85.09% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f99cb1f...98c6daf. Read the comment docs.

dirs:
- tests
# additional pytest arguments
pytest_args: -m "not pleasefixme and not torch_tts" --cpu --with_downloads --relax_numba_compat
Copy link
Member

Choose a reason for hiding this comment

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

@SeanNaren why is this? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe @ericharper could explain the additional parameters here, was forwarded it from the slack channel!

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering what the commands shall do --cpu --with_downloads --relax_numba_compat
when I call pytest --help none of them is there... do you need dome special extension?

Copy link
Member

Choose a reason for hiding this comment

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

@Borda I think you can define such things in a conftest.py file like here: https://github.com/NVIDIA/NeMo/blob/main/tests/conftest.py#L30

@Borda
Copy link
Member

Borda commented Jan 2, 2022

@SeanNaren the env for GPU is now set correctly, mind checking the failing tests?

.azure/testing-template.yml Outdated Show resolved Hide resolved
@Borda Borda enabled auto-merge (squash) January 3, 2022 11:22
@Borda Borda requested review from tchaton and a team January 3, 2022 11:22
@Borda
Copy link
Member

Borda commented Jan 3, 2022

I have fixed the lib install with docker but the tests are still failing...
But running test with --cpu option on GPU machine is likely useless... so let's drop GPU testing for now

@Borda Borda merged commit 2923817 into main Jan 3, 2022
@Borda Borda deleted the nemo branch January 3, 2022 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

integrate with Lightning ecosystem CI
4 participants