h-REA / hREA

A ValueFlows / REA economic network coordination system implemented on Holochain and with supplied Javascript GraphQL libraries
https://docs.hrea.io
Other
143 stars 15 forks source link

Details of update logic for EconomicResources? #65

Closed pospi closed 4 years ago

pospi commented 5 years ago

I've got some questions on how to manage the fields of an EconomicResource in regard to event-related functionality through the API, apologies if they are documented elsewhere; if not- this might be a good opportunity to describe the state machine at a high level.

When creating a resource via the additional createResource parameters in the createEconomicEvent GraphQL mutation:

I also have outstanding ambivalence about createResource as a parameter name in the event API, even though we've discussed that it's unavoidable. What if we renamed it to observedResource? I'd feel better about that :P

For updating a resource directly (pretty much only provided for correcting data entry errors...)

For updating a resource in response to an event:

One other unrelated thing- is onhandQuantity the correct casing, or should the 'H' be uppercase?

Oh, and- deleting records... is this possible? Or should they stick around, even if all the events that created them and interacted with them are deleted? Is it worth implementing extra logic to manage the cleanup?

fosterlynn commented 5 years ago

Initial responses, probably more discussion needed. And good questions, glad you are running into them. cc @ivanminutillo

stage: should this be provided as an input parameter, just start off as 'none', or should it take on the ProcessSpecification of the related EconomicEvent that resulted in the resource creation, if EconomicEvent.outputOf is set? Or inputOf? Or something else?

I don't think it should be input, although maybe we should think about that. Yes, ProcessSpecification of the EE.outputOf of resource creation. But also PS of the EE.outputOf of resource update too. I'm thinking about if it should be only "modify" actions; and what to do about a bunch of edge cases where accept/modify is mixed in with produce type things.... needs some more analysis.

unitOfEffort: similarly, must this be explicitly provided or does it come from the associated ResourceSpecification?

If explicitly provided, use that; else if in RS use that; else null.

and state comes from the event's action, if the action is pass or fail? If that's going in to the model... how does a "pass" expire for inspections that need periodical review? It doesn't feel right for it to "stick" to the resource...

I think it can stick to the resource until another pass/fail event comes along. A periodic review would provide that, as well as any re-test. Can you think of gotchas with that?

I presume accountingQuantity adjusts in response to e.resourceQuantity based on e.action.resourceEffect? (being either increment or decrement)

Most actions update accountingQuantity and onhandQuantity both, if - or + or -+, and if there is an inventoried resource. Exceptions:

are any other EconomicResource fields affected by or derived from fields of the associated EconomicEvent that is provided at creation time?

Don't think so. Although a move event would affect currentLocation, as could other events. I'm thinking we should add that to the event properties, will think of a name. Make sense?

Should the resource be linked to the event responsible for creating it somehow? I suppose we would transparently create the resource address as part of the transaction and inject that as resourceInventoriedAs in the stored event?

I think we need to add the resourceInventoriedAs to the event. I suppose if you stored the resource first, you would have that.

More to come....

fosterlynn commented 5 years ago

I also have outstanding ambivalence about createResource as a parameter name in the event API, even though we've discussed that it's unavoidable. What if we renamed it to observedResource? I'd feel better about that :P

I don't have strong feelings about it, but observedResource doesn't really say it needs to be created. newObservedResource ? newInventoriedResource ?

Are we missing a bunch of fields in the mutations API to mirror the new resource fields in 0.3, or are some omissions intentional? (lot, unitOfEffort, stage, state & containedIn)

No omissions are intentional. Except stage and state would not come from a direct update, as per above.

Is classifiedAs ever altered by events, or does it retain the value it began with? i.e. do I update r.classifiedAs in response to e.resourceClassifiedAs if e.resourceInventoriedAs is provided? If so, is e.toResourceInventoriedAs modified as well?

I think r.classifiedAs could be updated by an updateEconomicResource. I suppose we could also update it from e.resourceClassifiedAs? In both cases, just adding onto the list. I guess e.toResourceInventoriedAs should work the same.

Is onhandQuantity affected by e.resourceQuantity, or just accountingQuantity?

onhandQuantity too, per exceptions in last comment.

Does containedIn respond to any action types?

No. Do you think we will want that?

What about currentLocation?

Yes, per above comment.

Kinda repeats, but- what is the logic for how to update stage & state?

Same, from an event. LIke a resource got re-tested. Or a resource went through another stage.

One other unrelated thing- is onhandQuantity the correct casing, or should the 'H' be uppercase?

onhand should be one word, although I see my dictionary doesn't agree. :)

Oh, and- deleting records... is this possible? Or should they stick around, even if all the events that created them and interacted with them are deleted? Is it worth implementing extra logic to manage the cleanup?

Should be based on deletable method. Should be deletable if nothing has happened to connect them to anything, logic is specific to the type of record. For resources, can't delete without deleting the event that created them, those should stay in sync. I can spec out the logic for deletable methods.

pospi commented 5 years ago

I'm looking at the ResourceSpecification we have in currently, and it has no Unit metadata- looks to have moved to RecipeResource, but there's no linkage from there to the specification. Is that intentional, or are fields missing?

Other stuff...

I think it can stick to the resource until another pass/fail event comes along. A periodic review would provide that, as well as any re-test. Can you think of gotchas with that?

Not especially, I suppose it depends on whether you believe it falls to application logic to alert people when testing & other QA is due. Perhaps there should be a convention for usage of the event fields to indicate the next date necessary for assessment- before to indicate when the inspection needs to be redone?

I think r.classifiedAs could be updated by an updateEconomicResource

Done.

I suppose we could also update it from e.resourceClassifiedAs? In both cases, just adding onto the list.

I'm going to disagree with you there. For updating directly, it should replace (that endpoint is mostly for rectifying data entry mistakes). When it's a new event with a mismatching classification agree that it should be appended.

The one I didn't ask about with regard to this is EconomicResource.conformsTo, which only allows a single value. At the moment I have the event overwriting that if it differs but I am thinking it should probably be left as it was originally set and only updateable through the resource update API.

Does containedIn respond to any action types?

Hmm, maybe this doesn't apply so much anymore. I thought we used to have action types that essentially covered packing and unpacking resources into boxes.

On the location stuff, I think I've suggested this before as EconomicEvent.observedLocation- see https://github.com/valueflows/valueflows/issues/486.

A deleteable logic spec would be super helpful! (:

pospi commented 5 years ago

Something else- I'm just going to remove classifiedAs from the creation parameters for EconomicResource. Reason being it creates ambiguity and unnecessary complexity in handling EconomicEvent.resourceClassifiedAs- I would prefer to just be able to use that field as-is with the new resource, rather than having to concatenate and de-duplicate it.

pospi commented 5 years ago

Oh, and I forgot one of your other questions. I quite like newInventoriedResource, shall we go ahead and update that in the GraphQL spec?

fosterlynn commented 5 years ago

I quite like newInventoriedResource, shall we go ahead and update that in the GraphQL spec?

Sure.

I'm just going to remove classifiedAs from the creation parameters for EconomicResource.

Sounds good.

I'm looking at the ResourceSpecification we have in currently, and it has no Unit metadata- looks to have moved to RecipeResource, but there's no linkage from there to the specification. Is that intentional, or are fields missing?

It was intentional. My thinking was that ResourceSpecification would want to be more general, and that the unit was most important in the recipe itself (it is essential). And that people might want to use different units in their recipes which create the same thing. When people trade, the unit will probably be specified by the provider, and people will need to do conversions for themselves as needed, like pounds to liters or whatever. Note: we haven't tried this yet, so will need to pay careful attention to how it works with user groups and in the code.

I think it can stick to the resource until another pass/fail event comes along. A periodic review would provide that, as well as any re-test. Can you think of gotchas with that?

Not especially, I suppose it depends on whether you believe it falls to application logic to alert people when testing & other QA is due. Perhaps there should be a convention for usage of the event fields to indicate the next date necessary for assessment- before to indicate when the inspection needs to be redone?

I think alerts for regular maintenance etc. will fall to the planning layer and specific application logic. There will be many ways people want to do this, and I don't see it as part of the HoloREA framework.

I suppose we could also update it from e.resourceClassifiedAs? In both cases, just adding onto the list.

I'm going to disagree with you there. For updating directly, it should replace (that endpoint is mostly for rectifying data entry mistakes). When it's a new event with a mismatching classification agree that it should be appended.

OK agreed.

The one I didn't ask about with regard to this is EconomicResource.conformsTo, which only allows a single value. At the moment I have the event overwriting that if it differs but I am thinking it should probably be left as it was originally set and only updateable through the resource update API.

OK agreed, let's try it that way (only through resource api).

Does containedIn respond to any action types?

Hmm, maybe this doesn't apply so much anymore. I thought we used to have action types that essentially covered packing and unpacking resources into boxes.

We do have proposed actions like that. Let's wait until we have a real use case though. LearnDeep will need something like that.

On the location stuff, I think I've suggested this before as EconomicEvent.observedLocation- see valueflows/valueflows#486.

We added atLocation to event based on your suggestion, but I see it is in the TTL but not in the UML, will fix. In any case, I understood that was for the event location, not the resource location. I don't think we can assume those are the same right now, haven't had a real use case for it.

pospi commented 5 years ago

Alright, we're getting pretty close now!

On validations, should all the transfer-* events require a toResourceInventoriedAs if resourceInventoriedAs is provided? And do they always require both params, or can one log the transfer event types on the basis of specifications alone rather than actual inventoried resources?

On unit conversions, I like that idea in principle, but think it relates to QUDT as per https://github.com/valueflows/valueflows/issues/268#issuecomment-532510494.

This feels a bit unwieldly and is a consequence of the changes to the event / transfer architecture. Would be interested in your reflections...

Something on stage and state that might not have been considered is that we could implement those as "pull" rather than "push" by determining them at read time rather than syncing with event updates. The other stuff all comes from the input event, which is why it's simpler to implement.

I understood that event.atLocation was for the event location, not the resource location. I don't think we can assume those are the same right now, haven't had a real use case for it.

Ok that does seem sensible. Perhaps it could depend on the action? For me that would make sense, given the ideological shift in the way we're thinking about actions (i.e. moving from "we're not sure about verbs, they're up to each group" to "we're pretty sure we have covered every possible type of action, use one of these or suggest an addition to the spec").

pospi commented 5 years ago

RE deletion of resources, see https://github.com/holo-rea/holo-rea/commit/845ee3688c36ab09fc56e4c0b03f184eb32ae615. I think it should just be that if the resource creation event is deleted, the resource is also deleted. Thoughts?

fosterlynn commented 5 years ago

On validations, should all the transfer-* events require a toResourceInventoriedAs if resourceInventoriedAs is provided?

No, let's leave it flexible.

And do they always require both params, or can one log the transfer event types on the basis of specifications alone rather than actual inventoried resources?

Actual inventoried resources are never required. Even if agents do inventory those resources, in transfers often the VF independent view will include only the ResourceSpecification, not the identifiers of the agents' inventoried resources. These are there mostly for when we do actually need them in the independent view, such as many currency transfers.

Something on stage and state that might not have been considered is that we could implement those as "pull" rather than "push" by determining them at read time rather than syncing with event updates.

That is a legitimate option, especially since the data is usually relatively close at hand (not far back in the chain). All that VF says is that any defined piece of data should be served, but how it is stored (or not) will be dependent on mostly technical considerations, as far as I can see.

More on this and the rest later.

fosterlynn commented 5 years ago

Also see here https://github.com/valueflows/vf-apps/issues/4. We'll continue to work through some of the different logic in that set of issues, and eventually document them somewhere more formal.

pospi commented 4 years ago

https://github.com/valueflows/vf-apps/issues/4 is quite different from what we have in the GraphQL spec; namely that there isn't a "create resource flag"... it's a payload. Have things changed since we specced that out, or am I picking at semantics? :P

The key difference I'm seeing regarding quantities in our current approach is that you can specify initial values for the resource via accounting_quantity & onhand_quantity, which are then subsequently modified by the event's data. Should I remove those?

Will go ahead with implementing state via a "pull" query today- can't do stage yet, will probably have to complete Specification to finish the rest off. I think that's the better architecture for those fields- it means we don't have to deal with any of the complicated "process was modified partway through" logic or event editing rollbacks and can just query stuff at runtime.

fosterlynn commented 4 years ago

namely that there isn't a "create resource flag"

I thought we were going for "newInventoriedResource"? Thought that was a flag, maybe not? Whichever we use, the logic is "if in some way you can tell this is supposed to create a new resource....".

The key difference I'm seeing regarding quantities in our current approach is that you can specify initial values for the resource via accounting_quantity & onhand_quantity, which are then subsequently modified by the event's data. Should I remove those?

Yes, only events should be able to change resource quantities.

pospi commented 4 years ago

It's not for changing, just for initializing. So the way I have it set up now is like saying "I have 10 of this resource in my inventory and I just consumed 2 of them". Whereas without the quantities present in the resource initialiser you would have to say "I produce 10 resources"; "I consume 2 of them" - or potentially just "I produce 8 resources". If we leave those parameters in the resource initialisation block then users can do any of the above; if not they are limited to the latter 2 options.

fosterlynn commented 4 years ago

It's not for changing, just for initializing. So the way I have it set up now is like saying "I have 10 of this resource in my inventory and I just consumed 2 of them". Whereas without the quantities present in the resource initialiser you would have to say "I produce 10 resources"; "I consume 2 of them" - or potentially just "I produce 8 resources".

It would be kind of unusual to produce and consume some resources in the same process, although I guess I could imagine it happening. But then that never goes into inventory, and you would be producing 8 out of one process. So saying "I have 10 of this resource in my inventory" doesn't really make any sense as a way to create inventory.

But in any case, in general I don't see that we have a place for "initializing". Anything that does show up in inventory needs to have an event recorded for how it got there. (If people are just coming up on a software app, and need to input beginning balances, they can do that with the raise event.)

Hope I don't create confusion with this, but it is also possible to have inventory inside a process, which is not available to other processes. Like an herbal drying site, or cheese maker, will need to keep track of herbs or cheeses as they are processing. Or transportation, where the shipper needs to keep and provide an inventory of the resources in transit. But still, this is always created by events. And we'll have to try this out in a real use case, as we haven't had that in our software in older implementations. Maybe "inventory" is the wrong word for this case. And if this is confusing, please ignore for now and stick with the above.

pospi commented 4 years ago

I think some red herrings might be creeping in here :) When we spoke about creating resources (I think it might have been in the old Mattermost channel); we landed on the idea that by necessity we need to provide a payload to initially describe resources paired with the event that creates them. So there is still always an event recorded for how it got there; but the event itself is insufficient. The information in the payload I linked above (with the possible exception of quantities) can not be determined from any data in the event- it has to be extra. And I didn't like this reality and remember expressing that I'd like it to go away and all be done through the event one day, which makes it a Great Thing to be having this conversation. But we couldn't find a way to do it then. If we can now, I'm super happy.

fosterlynn commented 4 years ago

Oh no, not red herrings! :) Possibly it was how it was framed within a process that threw me off.

@bhaugen and I had a brief conversation at supper, then I "crashed and burned" as we call it, sorry. Which was that we should not allow changes to event quantities, also not allow deletion of events. All changes to quantities would have to be a further event, even to correct them. That would keep us more in line with generally accepted accounting principles, which is probably a good thing for this implementation. And we wouldn't have to worry at all about the effect of accounting period reporting and such. There are probably other properties that this would apply to also, will give that a think later - provider and receiver agents would also affect accounting reports for example.

I'm also good re-addressing what we can stuff into events vs resources.

pospi commented 4 years ago

Ok, that sounds easier so I'm happy to go with it.

But maybe it's not just resource_quantity and effort_quantity that you don't want to be editable... perhaps events just shouldn't be editable at all, either? Because if those shouldn't be updateable, then resource_conforms_to surely isn't meant to be updateable as well since the associated resource's unit_of_effort depends on that. Same for output_of interplaying with EconomicResource.stage.

From this I'm thinking that maybe events shouldn't be editable at all... treat them as an append-only log. Thoughts?

fosterlynn commented 4 years ago

In general: I think we want to allow edit on for example description, which can be used as detailed notes on someone's work - I forget the term used, maybe "lab notes"? It would be rude to have to back out a whole event just to clarify your description. So accounting vs non-accounting fields.

But to clarify: there is always an append-only log that is maintained by holochain, right? You can always get back to all changes? And holochain manages that and generally returns the most updated copy of something if you just ask for the record?

Suggest the fields that can be changed on event:

@pospi any of those seem problematic? We can cut it down further for our own reasons. @bhaugen do you disagree with any of these? (We had some discussion that is possibly not completely resolved about structural elements vs standard accounting elements.)

bhaugen commented 4 years ago

I don't disagree about what's changeable, but I am also ok with not allowing any changes for now, and going with the accountant's rules (back the sucker out (negative quantity) and then redo).

On Thu, Oct 17, 2019 at 7:05 AM Lynn Foster notifications@github.com wrote:

In general: I think we want to allow edit on for example description, which can be used as detailed notes on someone's work - I forget the term used, maybe "lab notes"? It would be rude to have to back out a whole event just to clarify your description. So accounting vs non-accounting fields.

But to clarify: there is always an append-only log that is maintained by holochain, right? You can always get back to all changes? And holochain manages that and generally returns the most updated copy of something if you just ask for the record?

Suggest the fields that can be changed on event:

  • note: String
  • atLocation: ID
  • agreedIn: URI
  • realizationOf: ID # Agreement
  • triggeredBy: ID # EconomicEvent

@pospi https://github.com/pospi any of those seem problematic? We can cut it down further for our own reasons. @bhaugen https://github.com/bhaugen do you disagree with any of these? (We had some discussion that is possibly not completely resolved about structural elements vs standard accounting elements.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/holo-rea/holo-rea/issues/65?email_source=notifications&email_token=AAA4VP4G4XQSFXIW3X5QNX3QPBIHPA5CNFSM4IXDTQYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBP3IEQ#issuecomment-543142930, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA4VP2HXRHBS5AX3WPX56DQPBIHPANCNFSM4IXDTQYA .

fosterlynn commented 4 years ago

I am also ok with not allowing any changes for now, and going with the accountant's rules

Seems like the problem is that we are dealing with more requirements than standard accounting, like the "lab notes" example. But also there are operational structures like I-P-O that probably want to just be correct without confusing extra records, and won't affect accounting reports (I think). Our EconomicEvents are more than just journal entries. But even so, I suspect that the note is actually the thing that will be most often changed, all the structural stuff is much harder to make mistakes on or be incomplete. I'm good with going with just note as changeable.

We had also discussed the possibility of splitting the records into one for can't-change and one for can-change. (Noting that everything is actually immutable, just a question of is it a new EconomicEvent or is it a Change Activity (in ActivityStreams speak, however that works in holochain).) To me this feels like too big a change for right now, and for our state of knowledge too. But it has possibilities.

I would think there are Commitments that also affect standard accounting reports, like feeding Accounts Receivable and Accounts Payable.

Is it worth taking a closer look at what is actually generally required for the kinds of accounting reports that are published and then considered a done deal? ("Books are closed.").... I took a quick look and didn't see anything that requires more than agent-resource(classification)-quantity-action-date, I think.

pospi commented 4 years ago

In answer to your above question @fosterlynn the one that is potentially problematic for me is atLocation when used with a move event, as this must update EconomicResource.currentLocation. But so long as the action can't be modified by an edit, I think that's fine- the update action still only needs the simple logic of setting that field on the associated inventory resource.

a question of is it a new EconomicEvent or is it a Change Activity (in ActivityStreams speak, however that works in holochain)

Agree that "Change Activity" sounds equivalent to the underlying DHT "update" entry(s) in Holochain.

pospi commented 4 years ago

No wait, after implementing it I can see how it's a problem. Causes issues if the edited event is not the latest event in the resource's provenance. So I guess make location non-editable?

fosterlynn commented 4 years ago

So I guess make location non-editable?

Agree.

pospi commented 4 years ago

The remaining stuff to deal with then is just a case of what is directly createable and editable on EconomicResource. As discussed above, a payload in addition to the EconomicEvent is necessary to initialise the resource fields. Currently what is implemented is:

For creation:

For update:

Currently, everything except stage is editable but I expect that needs to be dramatically cut back.

Please correct as needed!

fosterlynn commented 4 years ago

conforms_to is mandatory

Correct. [edited that answer, was forgetting we changed the meaning of conforms_to]

tracking_identifier, lot, image, current_location & note are optional

Correct.

accounting_quantity and onhand_quantity are also optional, but I think the decision was to remove them and only allow the values to come from the event?

Correct, quantities can only come from the event. I think if there is a resource created from an event, then at least accounting_quantity should be mandatory, unless you plan on calculating it from events each time (which I don't think is a good idea, but I don't know the hc implications).

unit_of_effort, if provided, will override that of event.resource_conforms_to.unit_of_effort

When we separated out ResourceSpecification from RecipeResource, the unit_of_effort went to RecipeResource, which has no relationship any more to the observation layer. There may be something in a Commitment. But in any case, yes, anything on the event is what should be used when creating the resource, you shouldn't need anything else.

For update: Currently, everything except stage is editable but I expect that needs to be dramatically cut back.

Do you mean update from an event, or update directly (with something like a create activity, or whatever holochain uses natively)? Assuming update directly, how about this:

  classifiedAs: [URI!]
  unitOfEffort: ID # Unit (would affect events going forward)
  image: URI # URI
  containedIn: ID # EconomicResource
  note: String

[edit: I changed the above after talking to @bhaugen . He pointed out that we should be able to re-create an essential resource from the economic events and so there shouldn't be overlap between the things that can just be updated directly and things that can be updated by an event.]

fosterlynn commented 4 years ago

Note I edited the above comment.

So, here's a list of what I think can be updated by an event:

  accountingQuantity: IMeasure
  onhandQuantity: IMeasure
  currentLocation: ID # SpatialThing
  state: ID # Action
  stage: ID # Processspecification

If not one of those two lists, then they have to back the resource out (change quantities to zero and provide an explanation) with another event and re-do. @pospi does this make sense? @bhaugen also please verify.

pospi commented 4 years ago

if there is a resource created from an event, then at least accounting_quantity should be mandatory

You mean resourceQuantity on the event?

I'm a bit unclear on how to handle that initialisation logic. And also how to determine whether to init accountingQuantity vs onhandQuantity... I'm not sure we have thought that through. The other question is- do I just start the values at 0 and let the events modify from there? Or does the resource just directly take on resourceQuantity from the event (without doing any math) when being initialised?

RE the further questions on updating resources, I'm talking about updating them directly (not via an event). There are fields that I expect need to be editable, but which aren't under the influence of events. My thoughts are that:

fosterlynn commented 4 years ago

if there is a resource created from an event, then at least accounting_quantity should be mandatory

You mean resourceQuantity on the event?

I meant accountingQuantity on the resource, actually, from resourceQuantity. If there is only effortQuantity on an event, then the resource isn't inventoried so won't be created.

I'm a bit unclear on how to handle that initialisation logic. And also how to determine whether to init accountingQuantity vs onhandQuantity... I'm not sure we have thought that through. The other question is- do I just start the values at 0 and let the events modify from there? Or does the resource just directly take on resourceQuantity from the event (without doing any math) when being initialised?

See https://github.com/valueflows/vf-apps/issues/4 for my (untested) pseudo-code on the logic. Of course, whatever works for you that creates the same results. :)

fosterlynn commented 4 years ago

conforms_to, tracking_identifier, lot, image, contained_in & note aren't changeable via events- they need to be editable on the resource directly.

conforms_to, tracking_identifier, lot - we were thinking safest if those require new events to zero the old resource, and create a new one - we should get some feedback if that is excessive for people

we may want to make unit_of_effort editable, since the only way of changing it currently is logging an event with resource_conforms_to set.

Yes, it's on the editable list.

current_location I think might also want to be editable? But another option is to force someone to log a "move" event for that, which might be better.

Again, safer to do the latter.

classified_as I'm not sure we have done correctly- currently it is appended to when an event is logged against the resource with resource_classified_as set. Do we want to be appending values like that? Or should the value on the resource only be modifyable by editing the resource directly, same as the other fields above?

Good point. I guess people might want to get rid of an old one, so maybe make it a directly replaceable list?

pospi commented 4 years ago

I think this is everything I need, thanks heaps! Quite tired now but will submit a PR for #68 tomorrow (: