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

Release v1.3: new DAG solver, better plotting & "sideffect" #1

Open
wants to merge 146 commits into
base: master
Choose a base branch
from

Conversation

ankostis
Copy link

Please read https://github.com/ankostis/graphkit/blob/master/CHANGES.rst.
It contains also 2 plots comparing the flowchart of the library before and after this PR.

were writing in text-mode in PY3. and failing as encoding error.
Tokens work as usual while solving the DAG but
they are never assigned any values to/from the operation's functions.

+ TC included.
+ Docs updated.\+ Added `modifiers` superclass to facilitate identification code.
+ refactored FunctionalOperation._compute().
+ _norm_kwargs:
  + use isinstance() instead of type-equality checks,
    to support new modier classes;
    + avoid excessive dict searches with local vars.
not really needed, better be explicit which modifier is searched.
receiving partial inputs, needed for other operations.
+ The x2 TCs added just before are now passing.
NOTE dict are not deterministic in <PY3.6.
So this commit would not improve determinism
in those pythons.

+ build: add `boltons` dependency for ndexedSet.
+ doc: mark all set usage if affect determinism.
+ e.g. see reproducibility problem in yahoo#14.
needed to refactor the new pruning algorithm.

- expected 2 TCs fail for yet-to-be-solved yahoo#24 & yahoo#25 bugs.
override intermediate data.

More changes only for newer pruning TCs:
+ refact(test): rename graph-->netop vars for results of compose(),
  to avoid of `graph.net.graph`.
+ Explain failure modes in v1.2.4 & this merged branch (yahoo#19 + yahoo#23).
not after compose().

+ All TCs pass ok.
+ NOTE this is not yet what is described in yahoo#21.
to pass +TC checking DeleteInst vary when inputs change.

- x4 TCs still failing, and need revamp of dag-solution.
+ Read the next doc-only commit to understand changes.
+ Renamed:
  + net.steps --> net.execution_plan.
  + (old)compile() --> _build_execution_plan()
  + _find_necessary_steps() --> (new)compile() + _solve_dag()
    compile() became the master function invoking _solve_dag &
    _build-execution_plan(), and do the caching.
+ refact(compute()): extract common tasks from sequential/parallel.
+ refact show_layers() to allow full-print, geting also string
  (not just printing), and using custom classes for representation.
+ Raise AssertionError when invalid class in plan.
  it's a logic error, not a language type-error.
Probaly unreported bug in v1.2.4 for '_neccessary_steps_cache`.
+ Pruning behaves correctly also when outputs given;
  this happens by breaking incoming provide-links
  to any given intermedediate inputs.
+ Unsatisfied detection now includes those without outputs
  due to broken links (above).
+ Remove some uneeded "glue" from unsatisfied-detection code,
  leftover from previous compile() refactoring.
+ Renamed satisfiable --> satisfied.
+ Improved unknown output requested raise-message.
+ x3 TCs PASS, x1 in yahoo#24 and the first x2 in yahoo#25.
- 1x TCs in yahoo#25 still FAIL, and need "Pinning" of given-inputs
  (the operation MUST and MUST NOT run in these cases).
numpy was used just for its assert_raise
so as to politely print IndexedSets in those attributes.
but still annotate edges with optional edge data-attribute.

Reverted bc Operation must not know its network, to belong to more than one.
Also the `Operation.net` contradicted `NetwotkOperation.net`;
the later indeed is the network it wraps
(not the net it is part of).
+ homegine titles
+ fix: autodoc renders nothing without :special-members:
+ enh(net): mark dag-edges with sideffects;
+ enh(plot): plot sideffect inks as such;
+ enh(plot): update legend;
+ doc(plot): move legend text along with the code producing it.
+ doc: stray image fixes.
if built without cleaing, stray artifacts might make it into wheel.
and when giving `pytest --lf` i get:

_____________________________________ ERROR collecting setup.py _____________________________________
/usr/lib/python3.7/distutils/fancy_getopt.py:233: in getopt
    opts, args = getopt.getopt(args, short_opts, self.long_opts)
/usr/lib/python3.7/getopt.py:93: in getopt
    opts, args = do_longs(opts, args[0][2:], longopts, args[1:])
/usr/lib/python3.7/getopt.py:157: in do_longs
    has_arg, opt = long_has_args(opt, longopts)
/usr/lib/python3.7/getopt.py:174: in long_has_args
    raise GetoptError(_('option --%s not recognized') % opt, opt)
E   getopt.GetoptError: option --lf not recognized
@ankostis
Copy link
Author

The new plotting code ... for the readme and the default behavior of the plot function, I'd prefer that it produces the barebones "structure" plot ... could add an "advanced" option

I believe the way plotting works is very close to what you described (minus the advanced flags),
just not described in detail.

You see, when you have built a NetworkOperation, its netop.plot() method prints a bare bone graph with just the 2 types of operation-nodes, and their links (no input/output data nodes):
bare graph

As soon as it has run, the same netop.plot() method delegates to the last plan's plot() method which prints
plan-no_solution

And if you add the solution into the mix, the data nodes change fill-color, and that's it.

Is that what you aked?

To introduce the more advance plotting functionality, it'd be nice to have a dedicated documentation page/section in the main docs site.

There is a dedicated section in the site (not a separate chapter), under the "intro" chapter.
I didn't put the above details in a separate chapter not to overwhelm or distract the user.
Can do bot if you wish.

I'm not comfortable with adding the sideeffect functionality for this PR.

Me neither.
I would prefer to iron out the base release and start building on top.
I will see when i will have time to do this.


I will come back later to your rest items.
But as i said, we have diverted the discussion from the original repo.
Can you move this discussion back there?
If there is something bothering you, you may reach my gmail (same account).

@ankostis
Copy link
Author

Regarding the repo choice, if you have troubles keeping it under the old company, what would you say to make a new GitHub-organization and move it there.

What do you think?

In the mean time, i'm pushing fixes to the main repo (yahoo#31).
Please continue reviewing it there. if it gets to move, all history would be in one place.

Tip:
When moving repos, the old url "silently" redirects to the new organization.

@syamajala
Copy link

Where is development moving to? I have my own graphkit fork as well. I'd consider migrating back to the original if it can provide the same functionality as my fork.

@huyng
Copy link
Owner

huyng commented Oct 23, 2019

we're moving it to this repo.

@syamajala
Copy link

What are the two repos here: https://github.com/pygraphkit

@ankostis
Copy link
Author

graphkit:
After I had asked you if you still have access to main repo and you didn't reply, i assumed you didn't,
and proceeded to plan B i had explained, setting up a new organization and moving repo here.
The repo here is not a fork by coincedence - i can delete it and fork in GitHub, to preserve the link,
and can add you owner to the organization - unless you still have full access to the main repo.

graphtik:
I needed the lib to be up'n running for my job. And i wanted to experiment with PY3.6 features only,
like contextvars. So a setup this one, which can function as an early adopter for future features.

  • It has readthedocs, code building & coverage all properly setup (check the badges).
  • The API has started deviating, eg, gone are the BW-compatible classes, the "parallel" method on the Operation is gone, and you no define the pool in a global ContextVar config dictionary for all networks. Read the CHANGES.
  • A major fix is that the eviction is "perfect", meaning that when outputs are asked, he soultion at the last execution-step contains just those outputs, with no need for extra filtering (there was an eviction case missing)
  • Contains some fixes in that would be nice to backort to py2 graphkit.

I'm open for suggestions on how to proced, but i need graphTIK to be on PyPi asap, bc i need it for my job.

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

Successfully merging this pull request may close these issues.

3 participants