Conversation
sgugger
left a comment
There was a problem hiding this comment.
Looks great, thanks a lot for taking care of it! Could you add the (great) demos you put in the presentation as tests in test_file_utils? That would be awesome :-)
|
Hmm, the PR/implementation itself is awesome. thank you, @lvwerra Looking at before and after examples I'm concerned with whether this doesn't make readability/easy-of-understanding worse than before. Perhaps this appears to be that way since I have been staring at the old way for a long time and the new code looks unfamiliar? I.e. is it the case of habituation bias? Thoughts? |
| with ContextManagers([]): | ||
| print("Transformers are awesome!") | ||
| # The print statement adds a new line at the end of the output | ||
| self.assertEqual(mock_stdout.getvalue(), "Transformers are awesome!\n") |
There was a problem hiding this comment.
btw we also have a context manager for catching std streams.
https://huggingface.co/transformers/master/testing.html#testing-the-stdout-stderr-output
scroll down to CaptureStd
no need to change - as either way works, just sharing we already have a built-in for that functionality.
There was a problem hiding this comment.
I'm not sure if this method hides the output from user being able to debug what's going on - as I just fixed CaptureStd not to hide output by default and only make a copy of it. #13803
again nothing needs to be done in this case as the output is trivial.
|
I think the better example is the code mentioned last, where there is a duplication of three lines. For a duplication of just one line, the benefit is not super visible indeed. |
|
I'm not at all sure the number of duplicated lines is the issue. With the way things are now the reader can instantly choose the relevant branch using the in-head processor and flow through as they read the code. With the addition of the context manager it is far more difficult to figure out what's going on. And if additional conditions are added as in the 2nd example in OP that makes the code even more difficult to follow. IMHO, this current line of work is mainly to make the maintainability better - e.g. to prevent cases where we fixed one copy of a branch and forgot the other - especially since they aren't even aligned the same. Would the following approach improve maintainability while keeping the readability easy? before: |
|
Not sure what's blocking this PR from getting merged @lvwerra ? It's just a tool that people are free to use or not and I personally would really like to use this in the Trainer to simplify all the code using autocast block in an if statement. The particular if is_amp_available:
with autocast(enabled=False):
some_code
else:
same_codeis present so many that I will create a with autocast_if_amp_is_available()
some_code |
What does this PR do?
This PR adds a
ContextManagersclass that wraps one, multiple or no context managers and acts as a single context manager as discussed with @sgugger and @stas00. This should reduce code duplications where the context is conditional e.g. can only be applied if a framework is used or flag is set.Example
The following example should illustrate how the
ContextManagersclass works:We can then either specify no, one, or more contexts:
The context managers will be nested in the
ContextManagersUse cases
With this class the following code snippets can be rewritten:
transformers/src/transformers/modeling_utils.py
Lines 767 to 773 in aea7c5b
Contexts with additional conditional statements are probably best dealt with like that:
transformers/src/transformers/modeling_utils.py
Lines 796 to 805 in aea7c5b
Another place where code duplication is even more severe is in upcasting added in #13573:
transformers/src/transformers/models/gpt2/modeling_gpt2.py
Lines 243 to 251 in aea7c5b
What do you think about this implementation?