skorch icon indicating copy to clipboard operation
skorch copied to clipboard

Loading extra arguments w/ cuda dependency on CPU

Open BenjaminBossan opened this issue 2 years ago • 0 comments

Supersedes #877

Description

This bug could occur if a user has set a parameter with a CUDA dependency and then tries to load the net without CUDA. Now, this works (again) as expected.

Details

The problem started occurring after PR #751, which introduced storing parameters set via set_params in the private attribute _kwargs. Normally, for attributes, we make sure that they can be loaded without CUDA, but attributes within _kwargs were not checked. Thus, loading those without CUDA failed. Unfortunately, this was not caught by CI because CI is not CUDA-enabled.

Implementation

The bugfix consists of making sure that we don't store any values in _kwargs. Since values are not needed, only the keys (parameter names), this is more efficient anyway. Thus, there are no more possibly CUDA-dependent values that can "slip through".

After discussion, we decided to also rename the attribute, as _kwargs was not very specific. The new attribute is called _params_to_validate and it is a set instead of a dict. Also, the _check_kwargs method was renamed to _validate_params and it doesn't take a kwargs argument anymore. And on top of that, it's changed the raised error from TypeError to ValueError.

The reason for making those change is that it now is similar to sklearn's _validate_params method on BaseEstimator (same signature and same error type). However, we don't make use of the actual sklearn machinery since our validation does a few things differently (e.g. proposing possible fixes when the name is wrong).

As the attribute was renamed, we would normally get an error when unpickling nets stored with the old attribute. To prevent this, we catch the old attribute _kwargs and convert it to the new attribute _params_to_validate.

Coincidental changes

  • Moved an entry in CHANGES.md to a different section
  • Added a reference to an existing entry in CHANGES.md
  • I adapted the code in hf.py to use the same new scheme

BenjaminBossan avatar Aug 01 '22 15:08 BenjaminBossan