Add the n_protocol_dag_results property to ProtocolResult

[ianmkenney]

This PR adds the n_protocol_dag_results property to the ProtocolResult class. It reflects the number of ProtocolDAGResults that were input to the _gather call.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (10fef9b) to head (6cdbb72).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #381   +/-   ##
=======================================
  Coverage   98.66%   98.67%           
=======================================
  Files          36       36           
  Lines        2097     2109   +12     
=======================================
+ Hits         2069     2081   +12     
  Misses         28       28           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ianmkenney]

I'm probably being a little (very) pedantic here, but since gather takes in an Iterable, which can include non-terminating generators, it's possible for gather to hang forever (before running out of memory and exiting).

Since I'm using the len function in this PR, even well-behaved generators will no longer work. An alternative would be to use list(protocol_dag_results), but this is still susceptible to the infinite generator case. I'm not sure what the best option for typing is, since there doesn't appear to be a good builtin that enforces a terminating iterable and I think defining and comparing to a custom type might be more extreme than just not allowing generators.

All of that said, I'm happy to revert the guardrail (and its test) if it seems like too much for this PR. Although I suspect type checkers will complain if I use len rather than list.

[dotsdl]

I think the case of infinite ProtocolDAGResults isn't remotely likely, but I see your point. We could slate changing the type hint for gather from Iterable to list for gufe 2.0, indicating that we explicitly expect a list (and therefore a finite number) of ProtocolDAGResults fed to gather.

Do you agree with this?

[dotsdl]

You will also need to make changes to the _to_dict and _from_dict methods in ProtocolResult reflecting these changes.

[IAlibay]

Also blocking on downstream testing and maybe a bit of a discussion on protocol needs.

[IAlibay]

I'm trying to wrap my head around the use case here, could you explain the case where None happens? I.e. at first glance setting None to zero seems incorrect if None means that the gather method did not pass through a value. In that case None means undefined and it's much cleaner than getting 0 with the idea that you got no results (but you do).

[ianmkenney]

I think in the context of a Protocol's gather, you'll never see a None passed in. I can see instances where ProtocolResults are used directly, incorrectly or not, and a None would make sense. But of course I immediately throw away the None in the code you highlighted above. I suppose there is still some indecision on my part what the right signature should be and which values the ProtocolResult holds on to. I'm tempted to just drop Optional[int] and use int with default 0.

Just from the tests, I see instances where someone would make a ProtocolResult directly and I'll probably do something similar in testing parts of stratocaster downstream:

class DummyProtocolResult(gufe.ProtocolResult):
    def get_estimate(self):
        return self.data['estimate']

    def get_uncertainty(self):
        return self.data['uncertainty']


class TestProtocolResult(GufeTokenizableTestsMixin):
    cls = DummyProtocolResult
    key = 'DummyProtocolResult-b7b854b39c1e37feabec58b4000680a0'
    repr = f'<{key}>'

    @pytest.fixture
    def instance(self):
        return DummyProtocolResult(
            estimate=4.2 * unit.kilojoule_per_mole,
            uncertainty=0.2 * unit.kilojoule_per_mole,
        )

[IAlibay]

Thanks @ianmkenney - is this a time critical thing? I would like to spend a bit of time to check what this means for the existing Protocols, but I don't think I'll have time until Monday :(

[ianmkenney]

Not necessarily. I'll be testing stratocaster strategies based on this branch until it's merged in the hopes that we get some solution that's close in spirit to what we have here.

[ianmkenney]

I think the case of infinite ProtocolDAGResults isn't remotely likely, but I see your point. We could slate changing the type hint for gather from Iterable to list for gufe 2.0, indicating that we explicitly expect a list (and therefore a finite number) of ProtocolDAGResults fed to gather.

Do you agree with this?

Yeah that works, I think in the long term it will simplify typing issues.

[dotsdl]

@IAlibay downstream do you serialize ProtocolResult objects, or ProtocolDAGResult objects? The former will be impacted by this change, but the latter will not.

[IAlibay]

@IAlibay downstream do you serialize ProtocolResult objects, or ProtocolDAGResult objects? The former will be impacted by this change, but the latter will not.

We serialize ProtocolResult objects - since this is facing a ton of users, we'd want to be able to deserialize any 1.x ProtocolResults - is that possible? I.e. could we add a try/except in from_dict?

[IAlibay]

How about this?

[IAlibay]

lgtm thanks!

[dotsdl]

@ianmkenney if you can resolve conflicts, I think this one is good to go for merge!