Skip to content

add pip_verbose option to PythonPackage and set it to True in PyTorch easyblock, to show build output when using pip#3065

Merged
migueldiascosta merged 3 commits intoeasybuilders:developfrom
Flamefire:20240103171205_new_pr_pytorch
Jan 8, 2024
Merged

add pip_verbose option to PythonPackage and set it to True in PyTorch easyblock, to show build output when using pip#3065
migueldiascosta merged 3 commits intoeasybuilders:developfrom
Flamefire:20240103171205_new_pr_pytorch

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

@Flamefire Flamefire commented Jan 3, 2024

(created using eb --new-pr)

This adds a new EC param pip_verbose defaulting to the value of --debug, which defaults to False.
In PyTorch this EC param is defaulted to True.

If set and pip install is used --verbose is appended to the install command.

@migueldiascosta
Copy link
Copy Markdown
Member

@Flamefire do you think we might want this as an option or even the default for other packages?

@Flamefire
Copy link
Copy Markdown
Contributor Author

@Flamefire do you think we might want this as an option or even the default for other packages?

We could add it as an option to PythonPackage similar to use_pip_editable, e.g. use_pip_verbose. However it currently already is enabled there when using eb --debug, so we already have that and IMO reasonable

Given all the trouble with PyTorch and to preserve the current behavior we have when switching from setup.py install to pip install I wanted to unconditionally enable it.

But of course the alternative is to make this an EC Param use_pip_verbose defaulting to None which means "use if --debug" and change the PyTorch Easyblock to just set that which might be better as it avoids potentially adding it twice (which is valid, but increases verbosity)

@migueldiascosta
Copy link
Copy Markdown
Member

But of course the alternative is to make this an EC Param use_pip_verbose defaulting to None which means "use if --debug" and change the PyTorch Easyblock to just set that which might be better as it avoids potentially adding it twice (which is valid, but increases verbosity)

I like that in general, but if there is a more immediate need of helping people debug PyTorch build issues I'm also ok with merging this PR right now

@Flamefire
Copy link
Copy Markdown
Contributor Author

I like that in general, but if there is a more immediate need of helping people debug PyTorch build issues I'm also ok with merging this PR right now

Then I'll update this PR as currently pip is NOT used in PyTorch but might be later especially with EB 5.x

@Flamefire
Copy link
Copy Markdown
Contributor Author

@migueldiascosta Done.

Note that I was a bit confused as we have e.g. pip_no_index -> --no-index and use_pip_editable --> --editable which is inconsistent.

I decided for pip_<foo> as it is shorter.

@migueldiascosta migueldiascosta changed the title Show build output of PyTorch when using pip add pip_verbose option to PythonPackage and set it to True in PyTorch easyblock, to show build output when using pip Jan 6, 2024
@migueldiascosta migueldiascosta changed the title add pip_verbose option to PythonPackage and set it to True in PyTorch easyblock, to show build output when using pip add pip_verbose option to PythonPackage and set it to True in PyTorch easyblock, to show build output when using pip Jan 6, 2024
'pip_no_index': [None, "Pass --no-index to pip to disable connecting to PyPi entirely which also disables "
"the pip version check. Enabled by default when pip_ignore_installed=True", CUSTOM],
'pip_verbose': [None, "Pass --verbose to 'pip install' (if pip is used). "
"Defaults to 'True' if the EB option --debug is used.", CUSTOM],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpicking but this isn't quite right, is it? (using --debug doesn't change the value of pip_verbose)

Wouldn't something like Pass --verbose to 'pip install' (if pip is used) even when EB option --debug is not used be more accurate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically you are right. It is rather the effect of this option that is enabled by default.
I don't think this is an issue as I can't see where the distinction matters and this is shorter in expressing what it does to users: If set to True --verbose is used. If set to False --verbose is not used. If unset --verbose is used if --debug is used. E.g. your wording would miss-describe the "False" case.

And if we want to be really strict about this then we'd need to change either the implementation or other descriptions, e.g. "Defaults to '.' for unpacked sources or the first source file specified", "Enabled by default when pip_ignore_installed=True", "Defaults to 'True' except for whl files", ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

your wording would miss-describe the "False" case

Indeed, I missed that

And if we want to be really strict about this then we'd need to change either the implementation or other descriptions, e.g. "Defaults to '.' for unpacked sources or the first source file specified", "Enabled by default when pip_ignore_installed=True", "Defaults to 'True' except for whl files", ...

Fair enough :)

Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta Jan 7, 2024

Choose a reason for hiding this comment

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

"Enabled by default", in that second example, is better than "Defaults to 'True'" though (if one interprets that what's enabled by default is the passing of --verbose, not pip_verbose itself)

Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta Jan 7, 2024

Choose a reason for hiding this comment

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

anyway, I admit this is really nitpicking, if nobody else does it before then, I'll merge this tomorrow even if there aren't any changes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking back and forth:

  • "Defaults to True" makes it clearer to the user (only observing effects, not internal state): If not set the value will be True which means --verbose is used.
  • But it is confusing to developers: The default is technically still "None", not True
  • "Enabled by default" might be clearer to developers that this option isn't changed although it could still be interpreted this way.
  • Additionally it might be confusing to users misinterpreting that --debug overwrites this (user set value) unless "something else" is done.

So both have drawbacks hence I guess it doesn't matter. Maybe we should rather be consistent and change the others to either way. Used your suggestion here for now

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're right.

(what about actually setting, as soon as possible, pip_verbose = True when --debug is used? Still not quite correct that it is the default, but at least a developer thinking about the internal state (after the value is set...) wouldn't make any wrong assumptions)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, pedantic mode off - will run some tests, merge if no issue comes up and open a separate issue about how to be consistent about this in general

Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta left a comment

Choose a reason for hiding this comment

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

lgtm

@migueldiascosta
Copy link
Copy Markdown
Member

Going in, thanks @Flamefire!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants