Scheduling notebook #66

galleon · 2021-10-05T07:54:29Z

Removing prints from code
Using async call to MZN so that it is usable in Notebooks
Telling right story for user

galleon · 2021-10-05T07:55:58Z

POPOGO · 2021-10-18T15:31:53Z

The cleaning of debug strings notably in discrete-optimisation part is interesting to include. The notebook is not ready so I can't approve this PR for now.

POPOGO · 2021-10-19T07:29:33Z

About the following bullet point :
Using async call to MZN so that it is usable in Notebooks
it is important that the current minizinc calls that we do in python scripts are working without making changes.
Maybe a way is to duplicate the solve function with a solve_async one.

dbarbier · 2021-10-20T06:47:37Z

skdecide/discrete_optimization/rcpsp/solver/cp_solvers.py

+    ParametersCP,
+    CPSolverName,
+    map_cp_solver_name,
+)


No! Do not reformat while making changes. This commit is unreadable.

You also put unrelated changes in commit 376afe8 (asyncio stuff in cp_solvers.py and the change in rcpsp_pile.py), you must move these changes in a seperate commit.

POPOGO · 2021-10-20T12:47:54Z

skdecide/discrete_optimization/rcpsp/solver/cp_solvers.py


        try:
-            result = self.instance.solve(timeout=timedelta(seconds=timeout),
-                                         intermediate_solutions=intermediate_solutions)
+            # Execute the solver in a separate thread


The change of solve() code to use a separated thread is the right trick to be able to use the CP Solver in Jupiter notebook for example. However, are we sure this doesn't interfere with the current way we are using the library (simple python script). I would prefer to have the choice between the two implementation via an attribute of the solver for example.

We may have a switch to call self.instance.solve or self.instance.solve_async.

Hmmm ... There is no solve_async. In fact, the problem is caused by Jupyter is running its own event loop. Many projects have faced the same problem and the simplest solution proposed is to call the function (here solve on the MZN instance in a separate thread.

I am happy to add a switch that would execute this function in the current thread or in a different thread but I am not sure who (developper or user) will trigger the switch. It seems to me that this might add at the end dead code.

yes the trigger would be between choosing to run self.instance.solve or _exec_async(...). So @galleon do you think your _exec_async is well named ? it seems it may be clearer if it was exec_in_other_thread if it's what it is doing really?

Hmmm ... There is no solve_async.

I was referring to https://minizinc-python.readthedocs.io/en/latest/api.html#minizinc.instance.Instance.solve_async but have no idea what to do with it.

In fact, the problem is caused by Jupyter is running its own event loop. Many projects have faced the same problem and the simplest solution proposed is to call the function (here solve on the MZN instance in a separate thread.

Ok but some tests now fail. not sure whether these failures are related to these changes.

OTOH with

import nest_asyncio nest_asyncio.apply()

at the top of the notebook, it seems to work just fine, but some people report performance problems with this solution.

Yes I have seen that ... (probably same SO thread) but I choose not to implement that because of:

the performance issue that was mentioned,

it is not robust for other users willing to test, start developing using Jupyter / JupyterLab

Need to have a look to the failing tests.

Ok so python 3.7 brings its own problem so I propose to use the patch nest_asyncio knowing that it is not a perfect solution as explained in this thread.
I also propose to make it explicit in the documentation and notebook so that users that want to use scikit-decide behind their own GUI or simply in Jypyter/JupyterLab know that they have to use that.

+1, sounds more realistic to me.

BTW it may be interesting to keep your thread idea in a different branch (rebased on this branch), in case we want to investigate it in the future.

POPOGO · 2021-10-20T13:21:25Z

Notebook comments :

-"Let's analyse the graph" cell : we could remove for now this cell if we don't have a super nice way of plotting the precedence graph

Astar solving :
we need to show what is the result, a way to do that is to use the same piece of code that are there exemples/scheduling/rcpsp_examples. Do a rollout, postprocess the last state of the rollout (containing all start and end date of the task, and plotting the resulting Gantt chart)
Experiments with SM files :
- rewrite the story as we thought of : start with the j30 file and show that CP returns optimal results versus A* can't handle it.
- Then try with the j12052_1.sm example as it's present currently in the notebook to show the advantage of using the LNS method

POPOGO · 2021-10-20T15:23:58Z

skdecide/discrete_optimization/rcpsp/solver/cp_solvers.py

+from concurrent.futures import ThreadPoolExecutor as _ThreadPoolExecutor
+
+
+def _exec_async(func, *args, **kwargs):


if this way of calling minizinc solver is the new way to go, it would be better placed at skdecide/discrete_optimisation/generic_tools/cp_tools as a public function with a simple name. It would help the implementation of other CP based solver (other problems than RCPSP) without having to replicate this small code everywhere.

dbarbier · 2021-10-22T09:06:08Z

Your commits are much easier to review now, thanks. You could also merge 4d81dc3 and 74a6451.
Please modify the commit message of 2a183d7 to explain why this change is needed.

dbarbier

I did not review run_cp.py, will this script be committed or is it an helper tool to write the notebook?

skdecide/discrete_optimization/rcpsp/solver/rcpsp_pile.py

dbarbier · 2021-10-25T06:05:42Z

skdecide/hub/solver/_hindsight/hindsight.py

-        if self.reuse_plans:
-            self.planned[next_state][key_scenario] = True
-        self.q_values[source][action] += (cost + cost_f) * self.weight_scenario[key_scenario]
-        return None


Commits 9dad62f, 0bbf6cc and f12cb70 could be merged. Since these changes are trivial to review, it would be a good idea to move them in a seperate PR.

@dbarbier Might be could to add a pre-commit hook to clean notebooks

There is one.

And do we have some documentation explaining how a dev can install all those hooks to get a warning when committing locally

You are putting the charrues before the boeufs, we first have to commit fixed files in master (without introducing conflicts in PRs). If developers run pre-commit before this task is done, this may cause conflicts.

dbarbier · 2021-10-25T06:12:28Z

notebooks/scheduling_demo.ipynb

+   "source": [
+    "import nest_asyncio\n",
+    "nest_asyncio.apply()"
+   ]


Please redo this commit without output cells to keep changes as small as possible.

dbarbier

ParametersCP.nr_solutions is described as "max number of solution returned". According to your new commit message, this description is wrong and should be fixed.

POPOGO · 2021-10-26T14:53:20Z

@galleon, i edited the notebook to address notably the third bullet point "Telling right story for user" and pushed to your branch. All can have a look to it :). We can have a last pass on it but we are close to the end I think

notebooks/scheduling_tuto.ipynb

dbarbier

LGTM but please squash all commits which modify notebooks/* into a single one to make history cleaner.

galleon · 2021-11-04T08:00:50Z

Commit history has been rewritten. The binder is provided but a new version will have to be issued as bug fixes are provided in the branch. The last solver will work only with a local install.

- Creating scheduling notebook tutorial - Adding appropriate sm data file - Patching asyncio in the Jupyter notebook to be able to use scikit-decide

galleon · 2021-11-05T10:47:30Z

Code changes have been moved to #97

galleon mentioned this pull request Oct 5, 2021

Work on #57 - Start of scheduling notebook example #59

Closed

galleon changed the title ~~Start work on scheduling notebook~~ Scheduling notebook Oct 8, 2021

dbarbier reviewed Oct 20, 2021

View reviewed changes

POPOGO reviewed Oct 20, 2021

View reviewed changes

galleon self-assigned this Oct 22, 2021

dbarbier suggested changes Oct 25, 2021

View reviewed changes

dbarbier mentioned this pull request Oct 25, 2021

Deployment of linters #84

Closed

10 tasks

dbarbier suggested changes Oct 25, 2021

View reviewed changes

nhuet mentioned this pull request Oct 25, 2021

Integration of new notebooks in doc #85

Closed

dbarbier mentioned this pull request Oct 26, 2021

gal/multi docs #72

Merged

POPOGO reviewed Oct 27, 2021

View reviewed changes

notebooks/scheduling_tuto.ipynb Outdated Show resolved Hide resolved

POPOGO reviewed Oct 27, 2021

View reviewed changes

notebooks/scheduling_tuto.ipynb Outdated Show resolved Hide resolved

nhuet mentioned this pull request Oct 27, 2021

ci-cd: add env variables needed to generate binder links in online doc #89

Merged

dbarbier approved these changes Nov 2, 2021

View reviewed changes

Work on #57

557e8ea

- Creating scheduling notebook tutorial - Adding appropriate sm data file - Patching asyncio in the Jupyter notebook to be able to use scikit-decide

g-poveda approved these changes Nov 10, 2021

View reviewed changes

g-poveda merged commit 934c8d5 into airbus:master Nov 10, 2021

galleon deleted the gal/scheduling branch November 20, 2021 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduling notebook #66

Scheduling notebook #66

galleon commented Oct 5, 2021 •

edited

Loading

galleon commented Oct 5, 2021 •

edited

Loading

POPOGO commented Oct 18, 2021

POPOGO commented Oct 19, 2021

dbarbier Oct 20, 2021 •

edited

Loading

dbarbier Oct 20, 2021 •

edited

Loading

POPOGO Oct 20, 2021

dbarbier Oct 20, 2021

galleon Oct 20, 2021

POPOGO Oct 20, 2021 •

edited

Loading

dbarbier Oct 20, 2021

galleon Oct 20, 2021

galleon Oct 22, 2021

dbarbier Oct 22, 2021

dbarbier Oct 22, 2021

POPOGO commented Oct 20, 2021

POPOGO Oct 20, 2021

dbarbier commented Oct 22, 2021

dbarbier left a comment

dbarbier Oct 25, 2021

galleon Oct 25, 2021

dbarbier Oct 25, 2021

galleon Oct 25, 2021

dbarbier Oct 25, 2021

dbarbier Oct 25, 2021

dbarbier left a comment

POPOGO commented Oct 26, 2021

dbarbier left a comment

galleon commented Nov 4, 2021

galleon commented Nov 5, 2021

		from concurrent.futures import ThreadPoolExecutor as _ThreadPoolExecutor


		def _exec_async(func, args, *kwargs):

Scheduling notebook #66

Scheduling notebook #66

Conversation

galleon commented Oct 5, 2021 • edited Loading

galleon commented Oct 5, 2021 • edited Loading

POPOGO commented Oct 18, 2021

POPOGO commented Oct 19, 2021

dbarbier Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

dbarbier Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

POPOGO Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

POPOGO commented Oct 20, 2021

Choose a reason for hiding this comment

dbarbier commented Oct 22, 2021

dbarbier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbier left a comment

Choose a reason for hiding this comment

POPOGO commented Oct 26, 2021

dbarbier left a comment

Choose a reason for hiding this comment

galleon commented Nov 4, 2021

galleon commented Nov 5, 2021

galleon commented Oct 5, 2021 •

edited

Loading

galleon commented Oct 5, 2021 •

edited

Loading

dbarbier Oct 20, 2021 •

edited

Loading

dbarbier Oct 20, 2021 •

edited

Loading

POPOGO Oct 20, 2021 •

edited

Loading