Skip to content

Conversation

@uribgp
Copy link
Contributor

@uribgp uribgp commented Dec 20, 2020

Related to #1496

Description: Breaking up large pull

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

uribgp and others added 30 commits December 15, 2020 17:51
This reverts commit 003956e.
@uribgp
Copy link
Contributor Author

uribgp commented Dec 20, 2020

It is returning this error:
error: Unsupported operand types for * ("int" and "None") [operator]

for this code in /ignite/ignite/engine/engine.py:
warnings.warn( "Data iterator can not provide data anymore but required total number of " "iterations to run is not reached. " f"Current iteration: {self.state.iteration} vs Total iterations to run : {self.state.epoch_length * self.state.max_epochs}" # type: ignore[operator] )

Here is the original:
warnings.warn( "Data iterator can not provide data anymore but required total number of " "iterations to run is not reached. " "Current iteration: {} vs Total iterations to run : {}".format( self.state.iteration, self.state.epoch_length * self.state.max_epochs, # type: ignore[operator] ) )

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 20, 2020

It's mypy complains about how you split the string. Annotation # type: ignore[operator] should be on the last string:

                            warnings.warn(
                                "Data iterator can not provide data anymore but required total number of "
                                "iterations to run is not reached. "
                                f"Current iteration: {self.state.iteration} vs Total iterations to run : {self.state.epoch_length * self.state.max_epochs}"  # type: ignore[operator]
                            )

@uribgp
Copy link
Contributor Author

uribgp commented Dec 21, 2020

It's mypy complains about how you split the string. Annotation # type: ignore[operator] should be on the last string:

Thanks. I have that locally, and my commit on github shows it that way. I believe it is the "autopep8 fix" which is formatting it that way for some reason.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 21, 2020

Thanks. I have that locally, and my commit on github shows it that way. I believe it is the "autopep8 fix" which is formatting it that way for some reason.

Yeah, I saw that... Can you find out who does that: autopep8 or black ?
I filed an issue to fix # type: ignore[operator] if possible (#1513 ). Maybe, we can simply create a substring with # type: ignore[operator] and finally concat it to the complete warning message...

@uribgp
Copy link
Contributor Author

uribgp commented Dec 21, 2020

It is autopep8

If I concat it, wouldn't it show up as a string?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 21, 2020

Here is what I was thinking about :

msg = f"Current iteration: {self.state.iteration} vs Total iterations to run : {self.state.epoch_length * self.state.max_epochs}"  # type: ignore[operator]

warnings.warn(
    "Data iterator can not provide data anymore but required total number of "
    f"iterations to run is not reached. {msg}"
)

@uribgp
Copy link
Contributor Author

uribgp commented Dec 21, 2020

Very clever, I'll give it a shot! #

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 22, 2020

autopep8 is unmercy :(
Let's try then the following:

total_iters = self.state.epoch_length * self.state.max_epochs  # type: ignore[operator]
msg = f"Current iteration: {self.state.iteration} vs Total iterations to run : {total_iters}"

warnings.warn(
    "Data iterator can not provide data anymore but required total number of "
    f"iterations to run is not reached. {msg}"
)

@uribgp
Copy link
Contributor Author

uribgp commented Dec 22, 2020

Yeah it's brutal! Giving it a shot.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 22, 2020

@uribgp please merge your branch with master. @gruebel fixed this in #1519.

@uribgp
Copy link
Contributor Author

uribgp commented Dec 22, 2020

OK, done.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @uribgp !

@vfdev-5 vfdev-5 merged commit 62f82c4 into pytorch:master Dec 22, 2020
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.

2 participants