deprecate ModuleAvailableCache with RequirementCache

[Borda]

Before submitting

• Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did all existing and newly added tests pass locally?

What does this PR do?

these two classes have almost identical code API and so it is confusing which shall be used when

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.

[carmocca]

This is not right. RequirementCache may check if the module is available but the functionality is different in the classes. I would agree with deprecating ModuleAvailableCache because we don't use it but the RequirementCache implementation should not be changed

[Borda]

I would agree with deprecating ModuleAvailableCache because we don't use it but the RequirementCache implementation should not be changed

So how can you use with RequirementCache("my.custom.subpackage")?
also, the base functionality does not change, it is extended...

[carmocca]

For that sort of check you have ModuleAvailableCache (stateful) or module_available (functional). There's no need to cram everything into a single class. If we find that checking for module paths is useful in a stateful manner then we shouldn't deprecate ModuleAvailableCache

[Borda]

well, I don't understand... on one side you are fine with deprecation but then not extending functionality such that you do use only the module path... 😖

[carmocca]

I'm proposing one of two paths:

• Deprecate ModuleAvailableCache, use module_available if you want to check whether a module is available. There's no use of ModuleAvailableCache in the lightning repository
• Do not deprecate ModuleAvailableCache.

[Borda]

Deprecate ModuleAvailableCache, use module_available if you want to check whether a module is available. There's no use of ModuleAvailableCache in the lightning repository

• yes, but we may not know if someone else is using it ins a project based on PL so we need to preserve functionality
• but module_available does not have cache so it is not the same equivalent

Do not deprecate ModuleAvailableCache.

so what is the difference when RequirementCache has almost the same functionality as ModuleAvailableCache so why not add an option just pass module the so the functionality overlap will be complete and can seamlessly use just one

[carmocca]

yes, but we may not know if someone else is using it ins a project based on PL so we need to preserve functionality
but module_available does not have cache so it is not the same equivalent

I agree with this analysis. Why deprecate it then?

so what is the difference when RequirementCache has almost the same functionality as ModuleAvailableCache so why not add an option just pass module the so the functionality overlap will be complete and can seamlessly use just one

It's not the same functionality, one checks for packages, other for modules. Just as we have package_available and module_available. There's no need to create a mix of both classes with if checks to differentiate what the user passed just for the sake of having a single class.

[Borda]

I agree with this analysis. Why deprecate it then?

The primary intention was not to deprecate but reduce variety when one class does 90%. Why not add the missing 10% instead of creating a special class for the 10% and duplicating 50% of the code
also, typing had a problem with

req = []
if a == 1:
  req.append(RequirementCache(...))
else:
  req.append(ModuleAvailableCache(...))

It's not the same functionality, one checks for packages, other for modules. Just as we have package_available and module_available.

so what RequirementCache has even argument module?

[carmocca]

You can annotate it with List[Union[RequirementCache, ModuleAvailableCache]] or you can introduce an interface that conforms to both and defines bool, and str

The module argument in RequirementCache is an optional last-ditch effort to check if the package is importable, to address issues where the requires() returns False but in reality the package was importable (example). Since the package name doesn't necessarily equal to the root module name, it had to be added. But it's not meant to be the principal mechanism to check whether a module is available. There's the other class/function for that.

[Borda]

You can annotate it with List[Union[RequirementCache, ModuleAvailableCache]] or you can introduce an interface that conforms to both and defines bool, and str

Sure, user is always welcome to write whole lit utils or lightning package, but

The module argument in RequirementCache is an optional last-ditch effort to check if the package is importable, to address issues where the requires() returns False but in reality the package was importable (example).

Cool, that is what I am saying, the feature is already there...

Since the package name doesn't necessarily equal to the root module name, it had to be added. But it's not meant to be the principal mechanism to check whether a module is available. There's the other class/function for that.

If requirement does not cover cases like sklesrn|scikit-learn we have design issue and shall not hack it extra module

Overall you are adding arguments why to do this change =)

[carmocca]

What do you think here @awaelchli? Would you put the functionality of ModuleAvailableCache and RequirementCache class together in one class where we choose what implementation to call based on some checks (as currently implemented), or would you keep them separate?

[awaelchli]

Afaik we needed to have separate functions module_available and package_available with quite different logic. We iterated many times to find the right way to implement these so I don't think we should put them together again. This separation seems to work well.

So I would say then it makes the most sense to also keep the classes separate.

If there is a concern for code duplication, maybe a better approach would be to have utility functions to share code, instead of using inheritance (when it makes sense, I haven't looked closely).

[Borda]

So I would say then it makes the most sense to also keep the classes separate.

that is fine with me but then we shall remove module argument from RequirementCache

[awaelchli]

This looks like it is used in several places for convenience. Removing it would break quite severely, since utilities is a core dependency of several Lightning packages. IMO none of these removals are worth the trouble for us and the users, this just looks like more work than it is worth it.

[Borda]

So to sumiraze, RequirementCache has a functionality which is half baked and you say we canot drop because people may use it and we canot fix/finish it becase you don't like it...

[Borda]

@carmocca could review this fix? 🐿️

[carmocca]

I dont have anything else to add to the discussion. I strongly suggest not continuing with this refactor. Is any bug or feature blocked by this PR?

Nothing else to add

[Borda]

I strongly suggest not continuing with this refactor.

Still the module argument is very confusing...