https://github.com/stitchfix/hamilton logo
Join Slack
Powered by
This message was deleted.
# hamilton-help
s
This message was deleted.
s
@Elijah Ben Izzy when you get up^
j
okay after a bunch of digging... i can see I at least have an import timing problem related to when i register my saver... wondering if this logic changed recently? previously i was managing to architect my imports so that I always had my saver imported early.... but I now I am really struggling to avoid importing 
hamilton.io.materialization
before my saver is registered seems this happens here i'm just trying to revert to 1.31 to see if that helps
s
yeah 
injectors
were pushed recently, I wonder if that changed some behavior.
j
ok yep 1.31 "works"
i picked on it only due to some references to jupyter notebook autocomplete stuff and some changes in that attribute setting stuff
and to sanity check, the error starts being thrown in 1.32
s
okay mind filing an issue to reproduce it?
👍 1
j
im not sure i can do an easy full repro, but ill try sketch it out abit
e
Hey, thanks for finding. Will look today. • this is an issue with imports and I think we can potentially improve it (I’ve been working around it locally but i think we can fix so it’s not necessary) • I can also add documentation to make it clearer in the meanwhile. Will respond in this issue!
FWIW the easiest/hackiest fix is to just stick an import before importing “to” (or in your mainline), but that’s potentially problematic for a few reasons, so I think we can make it so the import doesn’t matter, and maybe even avoid import.
Ask and you shall receive! I think I fixed this (it works in two unit tests): https://github.com/DAGWorks-Inc/hamilton/pull/523/files. Mind testing it out? Published an 
rc
version at 
sf-hamilton==1.37.2rc0
. Now, before accessing a “dynamic”property in the materializer, you’ll have to ensure register gets called. That either means calling it inline (uncommon), or importing a module that calls it in the body of the script (likely what you’re doing). I think the unit tests illustrates nicely:
Copy code
def test_dynamic_properties_can_be_registered_after_import_for_saver():
    @dataclasses.dataclass
    class CustomDataSaver(DataSaver):
        def save_data(self, type_: Type[Type]) -> Tuple[Type, Dict[str, Any]]:
            return "value", {}

        @classmethod
        def applicable_types(cls) -> Collection[Type]:
            return [dict]

        @classmethod
        def name(cls) -> str:
            return "testing_unique_key_saver"

    registry.register_adapter(CustomDataSaver)

    materialize_property = Materialize.testing_unique_key_saver
    to_property = to.testing_unique_key_saver
    load_from_property = save_to.testing_unique_key_saver
    assert materialize_property is not None
    assert to_property is not None
    assert load_from_property is not None
👍 1
j
i will try a bit later today .... one query, part of my problem is avoiding having 
to
imported before i register my saver.... is it still a bit of a race? i think that is implied in your test right?
oops and sry just saw your PR comment... sounds like i get to ignore order 🙂
e
Yep! Order shouldn’t matter — in the test above 
to
(and 
Materialize
, an internal construct) is imported out of scope. Registering just has to be done before calling to the property.
🙌 1
j
sry for the delay, this looks good on my side now... havent tested exhaustively but the fact im not immediately bombing out is good 😄
e
Great! Will release monday along with the next minor release (we’ve been doing a weekly cadence to add new features 🙂 )
👍 1
Open in Slack
PreviousNext
Powered by