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

Improve documentation of device-related methods and docs regarding GPU usage #1185

Merged
merged 6 commits into from
Aug 16, 2023
Merged

Improve documentation of device-related methods and docs regarding GPU usage #1185

merged 6 commits into from
Aug 16, 2023

Conversation

rajveer43
Copy link
Contributor

Close #909

paxcema
paxcema suggested changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@paxcema paxcema left a comment

Choose a reason for hiding this comment

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

Overall good start, thanks! However, we need to document more than just .amp. Please take a look at the helpers/device.py file, too.

lightwood/helpers/torch.py Outdated Show resolved Hide resolved
lightwood/helpers/torch.py Outdated Show resolved Hide resolved
lightwood/helpers/torch.py Outdated Show resolved Hide resolved
paxcema
paxcema reviewed Aug 9, 2023
View reviewed changes
lightwood/helpers/torch.py Outdated
"""
active = False

def __init__(self, enabled=True):
"""
Enters the context manager and enables AMP if it is not already enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a duplicate of __entry__, please fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still pending a fix.

paxcema
paxcema reviewed Aug 9, 2023
View reviewed changes
lightwood/helpers/device.py Outdated Show resolved Hide resolved
paxcema
paxcema suggested changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@paxcema paxcema left a comment

Choose a reason for hiding this comment

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

The new docstrings are great! We still need a few fixes before merging, I've highlighted them.

@paxcema
Copy link
Contributor

paxcema commented Aug 9, 2023 •
edited
Loading

@rajveer43 also, take a close look at the failing test. It provides details on where the linting process is failing. You should be able to reproduce this locally by running flake8 lightwood from the root lightwood folder path, assuming your python environment has flake8 installed.

@rajveer43
Copy link
Contributor Author

rajveer43 commented Aug 11, 2023 •
edited
Loading

I wonder why this deploy workflow skipped? any idea..would love to learn about it!

@rajveer43 also, take a close look at the failing test. It provides details on where the linting process is failing. You should be able to reproduce this locally by running flake8 lightwood from the root lightwood folder path, assuming your python environment has flake8 installed.

@rajveer43 rajveer43 requested a review from paxcema August 11, 2023 06:48
paxcema
paxcema reviewed Aug 11, 2023
View reviewed changes
lightwood/helpers/torch.py Outdated Show resolved Hide resolved
paxcema
paxcema suggested changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@paxcema paxcema left a comment

Choose a reason for hiding this comment

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

Some pending issues to fix before merging

@paxcema
Copy link
Contributor

paxcema commented Aug 11, 2023

To answer your question, the deploy workflow only triggers when merging into the stable branch. You can check the definition of these actions in the .github directory.

@rajveer43
Copy link
Contributor Author

rajveer43 commented Aug 15, 2023 •
edited
Loading

I was away for a long weekend! will do it asap! Can you tell me more issues to work upon of any repo of mindsdb, would love to contribute more!

@rajveer43 rajveer43 requested a review from paxcema August 16, 2023 05:27
@paxcema
Copy link
Contributor

paxcema commented Aug 16, 2023

Looks good, thanks for your contribution @rajveer43!

As for MindsDB, look for the good first issue tag in the mindsdb/mindsdb repo, or alternatively head to our community slack and ask over there, I'm sure we can help you find some other things to contribute on 😁

@paxcema paxcema merged commit 68d64d4 into mindsdb:staging Aug 16, 2023
6 checks passed
@paxcema paxcema mentioned this pull request Oct 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paxcema paxcema paxcema approved these changes

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

Successfully merging this pull request may close these issues.

Improve documentation of device-related methods and docs regarding GPU usage
2 participants
@rajveer43 @paxcema