feat: implement settings driven download and high level resolver customization

[shubhbapna]

fixes #191
fixes #185
fixes #184

[shubhbapna]

Have to update the overrides list

Edit: done

[tiran]

Config parsing and handling is getting a bit convoluted. We could look into Pydantic or attrs to do some validation, typing, and nice wrapping for us. I tried dataclasses in PR #241, but that's too limited. InstructLab uses Pydantic, but that's pretty heavy weight. attrs is a bit lighter.

PS: In a future PR, let's not delay this PR longer than necessary.

[dhellmann]

One real question and several nits for log messages.

[dhellmann]

I'm not sure this log message will help someone debug.

Suggested change

logger.debug(f"{req.name}: loading resolver parameters from settings")

[shubhbapna]

Well the hope is that it comes right before: https://github.com/python-wheel-build/fromager/pull/207/files#diff-e99a2837a7d36f9f14de5c4b54a1db688c08f148f62b8e3d5cb82937419dbca9R153

I was not able to cleanly combine these two statements hence the separate logs. Another option could be to log settings._get_package_settings and making that public

[dhellmann]

This might be a philosophical difference.

I generally want to log before something that may take some time, so if it looks like the program is "stuck" a user can reason about whether maybe it's just taking a while to compute or download something.

Then I also want to log when non-default behaviors are triggered, like an override is being used (or not).

This message is coming just before a lookup that should be very fast, and the results are logged later. So I think we can drop this message entirely and then fix up the phrasing of the other one and the user will have all the info they need.

[shubhbapna]

Yep we can drop the first log statement. If we want to fix the phrasing of the second log statement to indicate whether a value came from settings or not, then I will have to add additional checks just for that. It won't be as clean. If we want to indicate if it came from settings then I would prefer to do it they way we are discussing below

[shubhbapna]

@dhellmann regarding the logging, I think the way to go might be to log whatever settings._get_package_settings returns (so making that method public and calling it in download_source only for the purposes of logging)

[dhellmann]

@dhellmann regarding the logging, I think the way to go might be to log whatever settings._get_package_settings returns (so making that method public and calling it in download_source only for the purposes of logging)

How many different places will that function be called? Are we going to log the same info a lot?

Does logging all of the package settings at once make it easier to debug than just logging the values that are being used as they are used?

[shubhbapna]

How many different places will that function be called? Are we going to log the same info a lot?

just in the default_download_source function

Does logging all of the package settings at once make it easier to debug than just logging the values that are being used as they are used?

It will just log the package setting for the particular requirement we are working with currently. It might be useful so as to not surprise the user when the see a value that they were not expecting

[dhellmann]

Does logging all of the package settings at once make it easier to debug than just logging the values that are being used as they are used?

It will just log the package setting for the particular requirement we are working with currently. It might be useful so as to not surprise the user when the see a value that they were not expecting

But it would be all of the settings for that package, right?

[shubhbapna]

But it would be all of the settings for that package, right?

yes, correct

[dhellmann]

But it would be all of the settings for that package, right?

yes, correct

OK, let's do that. Ask for the settings, if we get any back log something like "found package build configuration settings" and dump all of them. Then later, just report things like "doing X using value Y" and don't report each time whether it's a setting, a CLI value, or the default.

[shubhbapna]

Done I removed the other log statements which logged the values because the resolver function and the download_url function already log them.

I just noticed 2 things as well:

• Should I override the resolver specific settings here as well: https://github.com/python-wheel-build/fromager/blob/main/src/fromager/sdist.py#L312
• Do the package names being passed to the methods in settings have to be canonicalized?

[dhellmann]

Done I removed the other log statements which logged the values because the resolver function and the download_url function already log them.

I just noticed 2 things as well:

• Should I override the resolver specific settings here as well: https://github.com/python-wheel-build/fromager/blob/main/src/fromager/sdist.py#L312

No, I don't think so. In that case we're looking at the wheel server we've been given to see if we've already built the wheel we're being asked to download. We only want to find a wheel, and we only want to look at that wheel server.

• Do the package names being passed to the methods in settings have to be canonicalized?

Yes. The values read from the file and the values from the Requirement both need to be canonicalized before you compare them or use them as keys in a dict.