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.

Sign up for GitHub

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

Jump to bottom

Fix bug #185 #200

Closed
wants to merge 0 commits into from
Closed

Fix bug #185 #200

wants to merge 0 commits into from

Conversation

nicoleeeluo
Copy link
Contributor

@nicoleeeluo nicoleeeluo commented Aug 12, 2024 •
edited
Loading

Description

  1. Commit to fix bug Update single_gpu_tutorial.ipynb to use a recent snapshot #185.
  2. Commit to fix bug single_gpu_tutorial.ipynb fails to run on GPU #183.
  3. Comment out optional result verification cells to avoid similar issues
  4. Update notebook for latest NeMo Curator version

Usage

Add snippet demonstrating usage

## Checklist
<!--
Note: All commits need to be signed and signed off. This can be done via `-sS` flags while commiting
`git commit -sS -m "...."
-->
- [ ] I am familiar with the [Contributing Guide](https://github.com/NVIDIA/NeMo-Curator/blob/main/CONTRIBUTING.md).
- [ ] New or Existing tests cover these changes.
- [ ] The documentation is up to date with these changes.

ryantwolf
ryantwolf requested changes Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @nicoleeeluo! I just have one small suggestion.

tutorials/single_node_tutorial/single_gpu_tutorial.ipynb Outdated
},
"outputs": [],
"source": [
"!export DASK_DATAFRAME__QUERY_PLANNING=False"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be changed to

%env DASK_DATAFRAME__QUERY_PLANNING False

And after doing that you should be able to remove the line below too:

os.environ['DASK_DATAFRAME__QUERY_PLANNING'] = 'False'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryantwolf Hi Ryan, fix updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nicoleeeluo can you delete the code instead of commenting it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryantwolf Thanks for pointing this out! I have deleted them

ryantwolf
ryantwolf approved these changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Thank you!

ryantwolf
ryantwolf requested changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@ryantwolf ryantwolf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Oops I missed a small error. Looks like a file (.pre-commit-config.yaml) got added that shouldn't be.

.pre-commit-config.yaml Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah wait I just noticed this. Can this change be removed? The PR should only modify the notebook

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryantwolf ryantwolf ryantwolf requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicoleeeluo @ryantwolf