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

rewrite for supply path logic #162

Closed
jdetaeye opened this issue Mar 8, 2017 · 22 comments
Closed

rewrite for supply path logic #162

jdetaeye opened this issue Mar 8, 2017 · 22 comments

Comments

@jdetaeye
Copy link
Member

jdetaeye commented Mar 8, 2017

after all model changes, the code is unnecessarily complex and should be revamped.

Core logic is fine, but the recursion data structure is not the best one any longer...

@jdetaeye jdetaeye self-assigned this Mar 8, 2017
@jdetaeye jdetaeye added the ready label Mar 8, 2017
@jdetaeye
Copy link
Member Author

Time to reprioritize this?

  • performance improvement with recursive query
  • review logic for the newly added auto-producing-flow. Current it issues a query per entity

@jdetaeye jdetaeye assigned hichamlahlou and unassigned jdetaeye Jul 19, 2019
@jdetaeye
Copy link
Member Author

#1091 also impacts this - suboperations can now also be defined from the operation table itself with a simple "owner" field

@hichamlahlou
Copy link
Member

@jdetaeye
If you have a wago model somewhere, I would be curious to know if this is faster than before.

@jdetaeye
Copy link
Member Author

Can't compare yet - the Wago model has many routings and also some alternates, and the new report doesn't come up correctly yet. To give an idea of depth and size of the supply path: between the "demand X asks" and "demand X gets answer" it is common that there are >1500 lines in the log file. Without the new log file structure I wouldn't have been able to solve the latest issue they found.

@jdetaeye
Copy link
Member Author

Wrote my previous commit without seeing your latest commit...
Will do a comparison right now.

@hichamlahlou
Copy link
Member

What is still not implemented is alternate operations (type=alternate), that piece of code is still missing but the logic should be quite close from the routings.

@jdetaeye
Copy link
Member Author

jdetaeye commented Jul 25, 2019

Quick test on Wago dataset https://wago.frepple.com/scenario16/supplypath/forecast/2002-1408%7C20021408_400019_40All%20customers/

Cloud-live:
image

New report:

  • committed fix for forecast supply path
  • doesn't open, but results of SQL query seem to match at first glance
    image
  • master query runs for 43 seconds. Not sure how much room for optimization there is.
    image

@hichamlahlou
Copy link
Member

Hmmm... clearly disappointing :-(
Will see where we can save some time.

@jdetaeye
Copy link
Member Author

jdetaeye commented Aug 13, 2019

On community branch (using the "demo" dataset) I see this case where the purchase operation isn't shown in the supply path:

image

Even when I specify the location it doesn't work correctly. -> Update: That is not correct. When filling the location in the itemsupplier records, i get the correct results.

@hichamlahlou
Copy link
Member

I tried few different things for this ticket.
Current committed code is preprocessing all operations with their bom in a cte but that can take up to 45 seconds to display the graph on huge models as seen on wago dataset.
Then I tried to write a sql starting from a buffer and looking for upstream objects so that I wouldn't have to build the huge cte described above.
Problem is it's almost impossible to have a single recursive query that goes thourgh the upstream objects considering that recursive queries in postgresql have limitations:

  • we cannot put a "union (all)" in the recursive part of recursive query (which is a problem as distribution and purchasing operations are not part of the operation table) so we need to use an OR which makes the query more complex.
  • we cannot use an aggregating function (jsonb_agg or array_agg) in the recursive part of a recursive query. It becomes also a problem when trying to fetch the bom of an operation into a single field.

Also, the fact that the producing record became optional in operation material tables adds complexity with plenty of coalesce here and there.
However, the new suboperation model using owner_id and priority made the query easier

I tried to write a query considering above constraints and, even before finishing writing it, it gave poor performance results on wago dataset.

bottom line is either

  • we stick to current commits (plus a couple of updates to fix problems found) => poor performance on large datasets

  • we keep current implementation that is faster on large datasets but still takes some time as een during wago workshop.

  • we preprocess the data as part of the supply planning (either we build the all_operations cte upfront or add a column upstream_operations at operation level) => extra data in the database, no refresh of the data unless a workflow has run.

@jdetaeye
Copy link
Member Author

I would rule out the third option - makes things difficult and unintuitive for newbie modellers. Not worth going that route.

Maybe a 4 fourth option: we could have 1 query per buffer, and let the view call&combine these.
This would overcome the limitations and complexity of recursive queries, and still reduce the number of database calls drastically from the original level.

For 6.0 release, I leave it to you to choose between options 1 or 2, or spend some more time on the 4th option.

@hichamlahlou
Copy link
Member

Put fix where item supplier location is null

@hichamlahlou
Copy link
Member

Not perfect but to me no more a showstopper if you want to move new django to cloud-live.
currently:

  • operations of type 'alternate' not supported.
  • slower on very large dataset (like wago).
  • does only work with new suboperation model (where parent is defined in operation.owner_id)
  • found a problem on the routings drawing on wago, only one suboperation is drawn. Not sure why as data is the same as with old code. It could be linked to the order of the records that are sorted differently.

What is left now is to rework the whole thing to have a query per visited buffer (operation?).

@hichamlahlou
Copy link
Member

Actually, thinking twice about it, the fact that this code only works with new suboperation model makes it a showstopper. It can only move with the 6.0.

@jdetaeye
Copy link
Member Author

jdetaeye commented Sep 1, 2019

Gives an error on wago model on this URL: https://wago.frepple.com/scenario8/supplypath/item/0221-0412%7C2210412/

image

@jdetaeye
Copy link
Member Author

jdetaeye commented Sep 2, 2019

On the wago model, the latest query runs for 222 seconds on my laptop...

image

@hichamlahlou
Copy link
Member

just pushed to community and enterprise a first version of the recursive supply path.

@hichamlahlou
Copy link
Member

I think it deserves testing
cannot do better than 5 seconds for complex wago supply paths like this:

image

Hexcel alternates are also supported, world record is this one with 27 alternate suboperations:

image

Had to add a security to stop recursion at some point. Problem rises when you go downstream from a raw material on a wago-like model. You might end up visiting all supply chain. Aligned on html, recursion stops at 400 nodes.

@hichamlahlou hichamlahlou removed their assignment Sep 4, 2019
@jdetaeye
Copy link
Member Author

jdetaeye commented Sep 4, 2019

Performance on wago dataset is like 20 seconds for this one.
The routings aren't displayed correctly here. The steps are numbered 5, 10 and 14 - could that explain it?

image

@hichamlahlou
Copy link
Member

Are you sure you have latest code ? It should show up in 5 seconds.
Suboperations look like this because they have not been correctly numbered:

image

@hichamlahlou
Copy link
Member

added a fix for incorrect numbering of suboperations

@jdetaeye
Copy link
Member Author

jdetaeye commented Sep 4, 2019

Looks good now. Time is about 7 seconds - better than what we had with the old generation of the report code.

@jdetaeye jdetaeye closed this as completed Sep 9, 2019
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

No branches or pull requests

2 participants