Hi Tammy,
Wow, that was pretty critical, and I'm not 100% sure how to react to it,
but I think there is a fundamental misunderstanding between what you
think is happening with Logic, and what is actually happening.
To re-iterate the key point: There is NO RE-WRITE of logic going on,
nor has there been a rewrite of Logic since 0.5. There should be
nothing changing at all in the Logic module that will affect you AT
ALL. If code was committed during the Calculation sprint that
contradicts this statement, that was a mistake, and should be caught
during code review and fixed before release. And if you want all of
that code removed, that's fine too - we could achieve the same results
with a calculation.logic module that exposes Logic rules as Calculations
as we would by adding the relevant adapter classes to Logic directly.
As the owner of the logic module, you are in control of Logic, and have
full authority to direct this as you see fit.. If you don't want Logic
to integrate with the Calculation module, you won't get any argument
from me - the point of this exercise from my perspective is so that I
will never have to use Logic again...
Thanks,
Mike
On 04/02/2012 03:00 PM, Darius Jazayeri wrote:
Hi Tammy,
My apologies for my part of the communication breakdown; as you say,
we've been trying to include you, but given that you've really been
out on maternity leave the entire time, that hasn't really worked...
That said, we have tried to take chica's needs into account, and I do
think that we've organized things in a way that makes chica /better/
off, and I look forward to discussing and/or clarifying on Wednesday's
design call. Are you available for that? (I haven't gotten a chance to
look at the changes to logic yet, but will try to do so before Wednesday.)
The genesis of it was that we started discussing "logic 2.0", and
realized that it would not be possible to switch to a new
lighter-weight design without hosing current logic users, so we
shifted to introducing the new design as the "calculation", and
leaving the current logic's design unchanged.
The plan we had laid out is:
* Chica will own logic, and have full ownership (and veto power)
over any changes. This largely means the 0.5 version you already
have running.
o Peeking at svn, the only changes since October have been:
(1) mavenizing logic, (2) switching it to use liquibase
instead of direct sql for its data model (hopefully you're
okay with maven+liquibase, but I guess it's up to you),
and (3) the changes that Wyclif committed.
* The intention of Wyclif's changes (which I haven't looked at
yet) is to add some /adapter/ classes to logic such that logic's
functionality can /also/ be accessed through the Calculation
interfaces, but this should have /zero/ impact on Chica's usage
of logic. I.e. the intention is that doing
logicService.eval("WEIGHT (KG)") will behave exactly as it does
now, and return a logic Result; and that an admin could also
configure things so that doing calculationService.eval("WEIGHT
(KG)") ends up calling that same underlying logic rule, and
getting back a calculation result.
o If the implementation of the adapters that Wyclif did
impose some new design or maintenance constraints on logic
(they shouldn't need to), we should fix that
So, to summarize, I completely agree that we want chica to own
/current/ logic (basically 0.5 + maven + liquibase), and I'd hope that
in the future you all add more functionality, and undo any bad
refactorings that we did there, but the main point is that there
should be no further changes to logic that breaks things for you,
because you'll be the gatekeeper.
AFAIK the only lines of code that Chica should need to change in the
short run are to go from Context.getLogicService() to
Context.getService(LogicService.class).
Further discussion on Wednesday.
-Darius
On Mon, Apr 2, 2012 at 11:02 AM, Tammy Dugan <[email protected]
<mailto:[email protected]>> wrote:
I finally had a chance to look at all the new logic code. First I
need to vent about a couple of things and then I will try to be
constructive:
* It took us MONTHS to get the last rewrite of logic to work
correctly with openmrs 1.7.x and the chica code
* We just got it working at the end of October 2011 and Openmrs
started rewriting it again in November 2011.
* The first round of design calls for the logic rewrite happened
in mid-November. I had a two week old baby and did not have time
to deal with it.
* Looking back through dev emails, the logic discussion ended at
the end of November and picked up in mid February
* In mid-February was the first time I saw anything about
Calculation.
* On March 5th there was a dev list email from Darius that he and
Mike would be here for one day and wanted to meet with me and some
other people
* I was still on part-time maternity leave but rearranged my
schedule with one days notice to meet with them
* When I met with them, I struggled to understand the logic design
that I had not looked at since I had only been back at work for 2
weeks after maternity leave
* In that meeting, Darius asked us if we could own logic.
Basically we were supposed to own something that got completely
redesigned that we had little or no input into the design that
didn't make any sense to us and stuff that had been fixed
repeatedly was broken yet again.
Based on all this, you can see why I might be a little irritated.
Yes, I know you tried to include me in the design but you knew I
was pretty much unavailable. Yes, I could have said no when Darius
said we should own logic but I felt backed into a corner. There
was a breakdown in communication on several fronts. If I would
have know we were expected to own logic, I would have said no to
the rewrite to begin with.
Venting done. Now to be constructive:
* The name Calculation is a horrible name for that object. It
doesn't calculate anything! Why not call it "RuleDefinition" since
it defines the properties of the rule and then call
CalculationEvaluator "Rule"? Why was Rule split into two objects
anyway? Why not just add a getParameterDefinitionSet method to the
rule interface? Why would you ever want some object that doesn't
evaluate anything being called by logic? What is the point in that?
* I think the strongly typed results and ParameterDefinitionSets
are nice changes
* Logic cannot hit the database for every token lookup. This is an
unacceptable performance hit for a system like chica. The tokens
need to be cached.
* CalculationEvaluators and Calculations should not be singletons.
Making them singletons is non-thread safe and requires special
programming that is not intuitive to the naive programmer. A new
instance should be created each time.
For a naive programmer, I think the new design complicates things
even more than before and makes it even less accessible for
developers to contribute.
In summary, if the chica group is going to own logic, we would be
willing to own the 0.5 logic that we worked so hard to getting
working. We are not willing to put the time and effort in to get
the new calculation version of logic working.
Thanks,
Tammy
On 3/29/2012 11:29 AM, Michael Seaton wrote:
Hi Tammy,
Is this an issue in the Calculation module? Or is this an issue
with how logic is exposing itself to Calculation? Weren't you
involved in the coding changes that took place during the
Sprint? That was meant to be for you and the Chica team to
contribute and own, not for anyone to impose on you.
It does seem we should cache calculation registrations in memory,
we are not doing this currently. But this shouldn't necessarily
be an issue for Chica, if it is going to the
logic_token_registration table for it's own purposes, should it?
I don't really see how exposing Logic to Calculation is a risk
for you, as long as you continue to access Logic natively as you
always have and just provide a couple of adapter classes to
expose it for other via the Calculation module.
Mike
On 03/29/2012 11:05 AM, Tammy Dugan wrote:
There were two major issues that came up when reviewing the
changes with Wyclif,
1. Logic cannot hit the database for every token lookup. This is
an unacceptable performance hit for a system like chica. The
tokens need to be cached.
2. Rules should not be singletons. Making them singletons is
non-thread safe and requires special programming that is not
intuitive to the naive programmer. A new instance should be
created each time.
I also want to point out that we have fixed #2 twice already in
previous versions and now it needs to be fixed for a third time.
Also, #1 was fixed in the previous version and now has to be
fixed again. It gets frustrating that these same problems keeps
getting reintroduced each time logic is refactored and each time
I have to make the case about why it should be that way and we
have to fix the problem. It is important when refactoring to
preserve the old behavior!
Because of these issues, we have no immediate plans to update
chica to use the new version of logic with Calculations. It took
us months to get things working before and we just don't have
the programming resources to do it now or in the near future.
Thanks,
Tammy Dugan
On 3/29/2012 10:08 AM, Friedman, Roger (CDC/CGH/DGHA) (CTR) wrote:
It appears that trunk produces 0.5.3 which includes
Calculation, last branch is 0.5.1 which is not mavenized, 0.5.2
is mavenized, can it be a branch even if not released?
*From:*[email protected] <mailto:[email protected]>
[mailto:[email protected]] *On Behalf Of *Wyclif Luyima
*Sent:* Friday, March 23, 2012 3:53 PM
*To:* [email protected]
<mailto:[email protected]>
*Subject:* [OPENMRS-DEV] Calculation sprint wrap up
Hi everyone,
For the past 2 weeks, we have been having a sprint on the
calculation module plus making logic and reporting to expose
themselves as calculations. We managed to get all the 27
tickets specific to the module done by Wednesday. Some work has
been done to retrofit logic and reporting to be exposed as
calculations, i went through the changes in logic with Tammy
and Win this afternoon, Tammy had some interesting points of
discussion which i believe she will send on the dev list.
In general, the code looks pretty good and i think we are ready
for its 1.0 release which could be in the next 1 -2 weeks,
apparently we have to wait for final high level reviews from
Darius and Burke as contributors to its design to confirm if it
is actually what they envisioned.
The sprinter turn up was good which included the core
developers, Mike and Mykola, thank you all for your relentless
work.
Have a great weekend.
Wyclif
------------------------------------------------------------------------
Click here to unsubscribe
<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l>
from OpenMRS Developers' mailing list
------------------------------------------------------------------------
Click here to unsubscribe
<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l>
from OpenMRS Developers' mailing list
--
Tammy Dugan
CHIRDL Technical Lead
Children's Health Services Research
IU School of Medicine
------------------------------------------------------------------------
Click here to unsubscribe
<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l>
from OpenMRS Developers' mailing list
------------------------------------------------------------------------
Click here to unsubscribe
<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l>
from OpenMRS Developers' mailing list
--
Tammy Dugan
CHIRDL Technical Lead
Children's Health Services Research
IU School of Medicine
------------------------------------------------------------------------
Click here to unsubscribe
<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l> from
OpenMRS Developers' mailing list
------------------------------------------------------------------------
Click here to unsubscribe
<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l>
from OpenMRS Developers' mailing list
_________________________________________
To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to
[email protected] with "SIGNOFF openmrs-devel-l" in the body (not
the subject) of your e-mail.
[mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
