python-wheel-build / fromager

Build your own wheels
https://pypi.org/project/fromager/
Apache License 2.0
3 stars 9 forks source link

feat: implement settings driven download and high level resolver customization #207

Closed shubhbapna closed 1 month ago

shubhbapna commented 1 month ago

fixes #191 fixes #185 fixes #184

shubhbapna commented 1 month ago

~Have to update the overrides list~

Edit: done

tiran commented 1 month ago

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.

shubhbapna commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

yes, correct

dhellmann commented 1 month ago

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 commented 1 month ago

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:

dhellmann commented 1 month ago

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:

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.