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

docs: change assert to raise and some small docs #26232

Merged
merged 6 commits into from
Sep 28, 2023
Merged

docs: change assert to raise and some small docs #26232

merged 6 commits into from
Sep 28, 2023

Conversation

pphuc25
Copy link
Contributor

@pphuc25 pphuc25 commented Sep 18, 2023

Hi,
I create a concise document outlining the process of improving code readability by replacing 'assert' statements with 'raise' statements, along with the addition of documentation and 'logger.info' statements.
I would like cc @stevhliu to review my PR.

@stevhliu
Copy link
Member

Hi, in general we like to use raise statements instead of assert because the latter are considered more like debugging statements that can be toggled on or off in certain setups.

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 25, 2023

So to catch more debugging information, I switch from using assert to raise. I believe the code I am working on is correct, @stevhliu.

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Sorry I read it the wrong way, you're correct! The rest of the changes look good to me. 🙂

Can you try running make style to fix the failing CI tests?

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 25, 2023

Hi @stevhliu, can you help me, I cannot run this command

@stevhliu
Copy link
Member

Yes, it looks like you're missing black which can be fixed by installing it first:

pip install -e ".[quality]"

Then you should be able to run the make style command :)

@pphuc25 pphuc25 requested a review from stevhliu September 26, 2023 05:56
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Thanks @pphuc25! Pinging @LysandreJik for a final look :)

Copy link
Member

@LysandreJik LysandreJik 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 for your changes @pphuc25, but let's focus on your changes from assert to raise statements for now.

Please revert the added comments/changes in logging statements; we want these scripts to be very minimal so we'll keep them the way they are currently for now.

Thank you!

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 27, 2023

Hi @LysandreJik, I have reverted them back. However, in run_mlm_no_trainer the logging do not have eval_loss, this seem the important one so user can know how well loss of their model is, I think we should keep add the eval_loss logging in that file.

@pphuc25 pphuc25 requested a review from LysandreJik September 27, 2023 10:25
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

I have written down the changes that should be reverted in my opinion, the rest (including the eval loss), can be kept!

Thanks!

@pphuc25 pphuc25 requested a review from LysandreJik September 27, 2023 10:51
Copy link
Member

@LysandreJik LysandreJik 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 for your PR!

@LysandreJik LysandreJik merged commit ba47efb into huggingface:main Sep 28, 2023
@pphuc25 pphuc25 deleted the raise_error branch September 28, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants