Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Steps cannot take Model as input #269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Steps cannot take Model as input #269

wants to merge 1 commit into from

Conversation

AkshitaB
Copy link
Contributor

@AkshitaB AkshitaB commented Apr 15, 2022

Right now, steps cannot take Model object as inputs, unless _to_params() is implemented. As it's unreasonable to expect users to write _to_params() for every implementation, we can add any empty implementation of it in the base class. However, that may lead to incorrect hashing for the object potentially?

Note: the actual layers in the model don't matter.

Error observed:

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../tango/common/testing.py:87: in run
    run_name = _run(
../../tango/__main__.py:707: in _run
    run = workspace.register_run((step for step in step_graph.values()), name)
../../tango/workspaces/local_workspace.py:345: in register_run
    all_steps = set(targets)
../../tango/step.py:452: in __hash__
    return hash(self.unique_id)
../../tango/step.py:431: in unique_id
    self.unique_id_cache += det_hash(
../../tango/common/det_hash.py:128: in det_hash
    pickler.dump(o)
../../../../../virtuals/tango/lib/python3.8/site-packages/dill/_dill.py:498: in dump
    StockPickler.dump(self, obj)
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py:485: in dump
    self.save(obj)
../../tango/common/det_hash.py:102: in save
    super().save(obj, save_persistent_id)
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py:558: in save
    f(self, obj)  # Call unbound method with explicit self
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py:884: in save_tuple
    save(element)
../../tango/common/det_hash.py:102: in save
    super().save(obj, save_persistent_id)
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py:558: in save
    f(self, obj)  # Call unbound method with explicit self
../../../../../virtuals/tango/lib/python3.8/site-packages/dill/_dill.py:990: in save_module_dict
    StockPickler.save_dict(pickler, obj)
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py:969: in save_dict
    self._batch_setitems(obj.items())
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py:1000: in _batch_setitems
    save(v)
../../tango/common/det_hash.py:102: in save
    super().save(obj, save_persistent_id)
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py:537: in save
    pid = self.persistent_id(obj)
../../tango/common/det_hash.py:107: in persistent_id
    det_hash_object = obj.det_hash_object()
../../tango/common/from_params.py:838: in det_hash_object
    r = (self.__class__.__qualname__, self.to_params())
../../tango/common/from_params.py:820: in to_params
    return Params(replace_object_with_params(self._to_params()))
../../tango/common/from_params.py:809: in replace_object_with_params
    return {key: replace_object_with_params(value) for key, value in o.items()}
../../tango/common/from_params.py:809: in <dictcomp>
    return {key: replace_object_with_params(value) for key, value in o.items()}
../../tango/common/from_params.py:809: in replace_object_with_params
    return {key: replace_object_with_params(value) for key, value in o.items()}
../../tango/common/from_params.py:809: in <dictcomp>
    return {key: replace_object_with_params(value) for key, value in o.items()}
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

o = Sequential(
  (0): FeedForward(
    (linear): Linear(in_features=4, out_features=4, bias=True)
    (activation): ReLU(...)
  )
  (2): FeedForward(
    (linear): Linear(in_features=4, out_features=4, bias=True)
    (activation): ReLU()
  )
)

    def replace_object_with_params(o: Any) -> Any:
        if isinstance(o, FromParams):
            return o.to_params().as_dict(quiet=True)
        elif isinstance(o, (list, tuple, set)):
            return [replace_object_with_params(i) for i in o]
        elif isinstance(o, dict):
            return {key: replace_object_with_params(value) for key, value in o.items()}
        elif isinstance(o, Path):
            return str(o)
        elif o is None or isinstance(o, (str, float, int, bool)):
            return o
        else:
>           raise NotImplementedError(
                f"Unexpected type encountered in to_params(): {o} ({type(o)})\n"
                "You may need to implement a custom '_to_params()'."
            )
E           NotImplementedError: Unexpected type encountered in to_params(): Sequential(
E             (0): FeedForward(
E               (linear): Linear(in_features=4, out_features=4, bias=True)
E               (activation): ReLU()
E             )
E             (1): FeedForward(
E               (linear): Linear(in_features=4, out_features=4, bias=True)
E               (activation): ReLU()
E             )
E             (2): FeedForward(
E               (linear): Linear(in_features=4, out_features=4, bias=True)
E               (activation): ReLU()
E             )
E           ) (<class 'torch.nn.modules.container.Sequential'>)
E           You may need to implement a custom '_to_params()'.

../../tango/common/from_params.py:815: NotImplementedError

@AkshitaB AkshitaB mentioned this pull request Apr 15, 2022
5 tasks
@AkshitaB AkshitaB requested a review from epwalsh April 19, 2022 21:06
@dirkgr dirkgr self-assigned this Apr 28, 2022
@dirkgr
Copy link
Member

dirkgr commented Apr 28, 2022

Note to self: Can we get rid of to_params completely?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants