Using `format/2` with attributed variable on a pengine

[jan]

@ridgeworks

When you have a chance, please review what’s there and let me know if there’s anything more to be done at my end to move this forward.

It runs on a local SWISH installation I has a first read through the code. These are some assorted comments

• in ia_primitives.pl we find a rather misleading definition of non_empty/1 and non_empty/2. The second looks like a mis-typed version of the first, but it probably is not because it is not called due to the goal_expansion/2 rule above. There should not be a global goal expansion/2 rule for (I think) a local predicate. As is, that will upset any code that happens to use non_empty/2. If it is only local, simply define the goal expansion rule in the module (remove the system:). Expansion follows a pipeline: first expand in the module being compiles, then in user and finally in system. The last should normally not be used by application code.

• The SWISH toplevel writes constraints for internal variables. I guess you can get rid of these using the SWISH post hook and remove the attributes from the variables that are not part of the bindings. Alternatively, leave them there, but extend the attribute on the variables in the bindings with an annotation that says they are part of the bindings.

• simplesolveall_/2 doesn’t seem to need the meta-calling route. There are only three predicates choice_generator_/6 can create which you then call. Seems choice_generator_/6 can simply make the call, no?

• Intercepting the tracer is something that should not be done by user libraries. Managing multiple interceptors is poorly defined and both the GUI debugger and SWISH use this hook (and cooperate poorly). It only seems to be used to wrap doNode_/7. That should be easy enough to wrap using normal Prolog. To achieve a zero-cost dynamic wrap you can use wrap_predicate/4. Doesn’t work very nice in the multi-threaded SWISH environment though.

• The {} definition is a no-op. This is a different predicate from {...} and not exported. Note that empty {} is simply undefined in most other subsystems using it.

• I’m a bit unsure about the usage of debug/1 and debug/3 in the code. Should this type of library debug (trace?) code use the generic debug facilities or their own? I think there is something to be said for both.

• What is the reason for set_min_free/1?

• clpStatistics/0: cut on last call is a no-op. It is not clear why you want to redo the work wrt. Prolog’s time and GC statistics anyway. Why not call call_time/2. Possibly time/1 should also print GC time?

I think that is enough for round one It is a nice library to have, especially if we can make it blend into the system cleanly such that it can safely and transparently used in larger projects.

[ridgeworks]

Thanks for testing on a local SWISH and for taking the time to review the code. Comments below:

@jan

• in ia_primitives.pl we find a rather misleading definition of non_empty/1 and non_empty/2. The second looks like a mis-typed version of the first, but it probably is not because it is not called due to the goal_expansion/2 rule above. There should not be a global goal expansion/2 rule for (I think) a local predicate. As is, that will upset any code that happens to use non_empty/2. If it is only local, simply define the goal expansion rule in the module (remove the system:). Expansion follows a pipeline: first expand in the module being compiles, then in user and finally in system. The last should normally not be used by application code.

There are two versions: non_empty/1 and non_empty/2; both are local to the module, and both are used (subject to goal expansion). So the basic idea was to test whether goal expansion was available on the system (I didn’t want to make the module dependent on that feature) using:
current_predicate(system:expand_goal/2). If so perform a goal expansion on non_empty/2, otherwise use non-empty/2 as defined. I didn’t think I was doing any “global expansion” and this is only applied to the module. Why is this not the case?

@jan

• The SWISH toplevel writes constraints for internal variables. I guess you can get rid of these using the SWISH post hook and remove the attributes from the variables that are not part of the bindings. Alternatively, leave them there, but extend the attribute on the variables in the bindings with an annotation that says they are part of the bindings.

If I understand you correctly, I currently do this for the normal top level using the user:expand_query and user:expand_answer that work with attribute_goals//1. It sounds like there’s something similar for SWISH (post hook?) that can be used to do this in a SWISH environment. Any doc or examples for this?

@jan

• simplesolveall_/2 doesn’t seem to need the meta-calling route. There are only three predicates choice_generator_/6 can create which you then call. Seems choice_generator_/6 can simply make the call, no?

I don’t think so. choice_generator_/6 tests if the interval can be sub-divided. If it fails, the second clause terminates the “search”. If it succeeds, a cut removes the second clause, but that cut must precede the meta call to Choices. So that call can’t be moved into choice_generator_/6.

@jan

• Intercepting the tracer is something that should not be done by user libraries. Managing multiple interceptors is poorly defined and both the GUI debugger and SWISH use this hook (and cooperate poorly). It only seems to be used to wrap doNode_/7. That should be easy enough to wrap using normal Prolog. To achieve a zero-cost dynamic wrap you can use wrap_predicate/4. Doesn’t work very nice in the multi-threaded SWISH environment though.

Didn’t know about wrap_predicate/4 (it isn’t in the “Summary” list of predicates; prolog_trace_interception/4 is). But I’ll put it on my TODO list if it can achieve a “zero-cost dynamic wrap”.

@jan

• The {} definition is a no-op. This is a different predicate from {...} and not exported. Note that empty {} is simply undefined in most other subsystems using it.

Not sure why that’s there - probably an historical artifact and should be removed.

@jan

• I’m a bit unsure about the usage of debug/1 and debug/3 in the code. Should this type of library debug (trace?) code use the generic debug facilities or their own? I think there is something to be said for both.

If you’re referring to the box model debugger, I found that was far too fine grained to be generally useful. Debug topics which enable additional messages on significant events, including interval watchpoints, and selectively enable noisy failures seem to work better for me and, I would think, most users who aren’t interested in the fine grained detail. I think it looks similar to the usage of debug topics in the sandbox module. But maybe I’m missing your point.

The whole issue of how to debug CLP programs is still a bit of an open question to me.

@jan

• What is the reason for set_min_free/1?

To be honest, I’m not quite sure anymore. I think the original strategy was to ensure, after garbage collection, there was sufficient stack space to run the fixed point iterator long enough to complete before hitting another gc. No guarantees of course, but seemed to be a useful heuristic and the number used was small enough that it was unlikely to have a significant global impact. Any recommendations?

@jan

• clpStatistics/0: cut on last call is a no-op. It is not clear why you want to redo the work wrt. Prolog’s time and GC statistics anyway. Why not call call_time/2. Possibly time/1 should also print GC time?

I believe the cut was originally present because the clpStatistics/0 predicate is discontiguous so it leaves an unwanted choicepoint; the cut was on the last clause to remove it. I’ve just tested it without the cut and it seems this is no longer the case (if it ever was ).

I did want a single “statistics” predicate that combined a few relevant systems numbers (time, inferences, stack usage, …) with clpBNR numbers (primitive ops, primitive failures, constraint network size). If I had found a “hook” to add my own operational measurements to the systems, I might have done it differently.

[jan]

@ridgeworks

So the basic idea was to test whether goal expansion was available on the system

I see. I misread, assuming that if you test for it in system you also define the system version. Yes, it is ok. I don’t think I’d go into all that trouble and instead would simply add a goal_expansion/2 rule. Systems that do not support it will just ignore it.

@ridgeworks

If I understand you correctly, I currently do this for the normal top level using the user:expand_query and user:expand_answer that work with attribute_goals//1. It sounds like there’s something similar for SWISH (post hook?) that can be used to do this in a SWISH environment. Any doc or examples for this?

So, it works differently. Inside the Pengine thread there is a call to swish_trace:post_context/1 that is normally not defined. The attribute_goals//1 hook is called in the HTTP thread that communicates between the Pengine and the HTTP client. The post_context hook receives a dict with quite a bit of context, among which the Bindings, a list of Name=Term for the answer. The hook could combine this answer to rewrite the attributes such that attribute_goals//1 in the HTTP thread does the right thing. Note that you can safely modify the attributes because the modifications are subject to backtracking anyway, so all modifications are undone in case the user asks for another solution.

To play, I’d make a local installation. That is quite simple:

 git clone https://github.com/SWI-Prolog/swish.git
 cd swish
 make yarn-zip
 swipl run.pl

That does not include clpBNR. I just added the config-available for that, so

 mkdir config-enabled
 cd config-enabled
 ln -s ../config-available/clpBNR.pl

(and restart swipl run.pl)

@ridgeworks

I don’t think so. choice_generator_/6

You are probably right. You can easily remove the meta call though by having a new predicate with three clauses. that calls the right continuation. That is probably as fast and avoids the need for sandbox declarations.

@ridgeworks

But I’ll put it on my TODO list if it can achieve a “zero-cost dynamic wrap”.

Great. As is, I don’t feel comfortable with this on a public server.

@ridgeworks

If you’re referring to the box model debugger

No. I am wondering whether a library that presents some form of debug messages to its user that are not there to debug the library, but to debug user code should use debug/1 and debug/3. Alternatively it could use its own debug statements and conditions. But, surely one could also argue this is the right way to go. I’ll have a look at handling debug messages from libraries in SWISH. There is not only the threading issue, but also whether or not this would allow to get access to information that should remain hidden. For example, library(pengines) uses debug messages, some of which reveal data about other Pengines.

@ridgeworks

Any recommendations?

Remove it If it leads to significant performance degradation there is something wrong with the garbage collector scheduling that should be fixed elsewhere. Such problems did exist, but several have been resolved.

@ridgeworks

(if it ever was ).

Nope. There is never an open choicepoint for a last call. There used to be one in old systems for dynamic predicates, but since we have the logical update view, there is not. Discontiguous would not be a good reason anyway. Multifile could be, as one could load a new file, but for SWI-Prolog. mulfile predicates also use the logical update view (in fact, all predicates do that, including static predciates).

[ridgeworks]

@jan

To play, I’d make a local installation. That is quite simple:

I managed to follow this and get a local copy running - I guess I
can handle copy/paste even when I don’t know what I’m doing. My problem seems to be how to define the hook. I tried:

:- use_module(library(swish_trace, [swish_trace:post_context/1])).

swish_trace:post_context(Dict) :-  %%% for swish testing
    Body...

but got an error:

% Updating GIT version stamps in the background.
ERROR: /Users/rworkman/.local/share/swi-prolog/pack/clpBNR/prolog/clpBNR/ia_utilities.pl:86:
ERROR:    source_sink `library(swish_trace,[swish_trace:post_context/1])' does not exist
Warning: /Users/rworkman/.local/share/swi-prolog/pack/clpBNR/prolog/clpBNR/ia_utilities.pl:86:
Warning:    Goal (directive) failed: clpBNR:use_module(library(swish_trace,[swish_trace:post_context/1]))

and if I don’t try to use it, it looks like the hook is 'abolished` later in the server startup:

Warning: /Volumes/Mac Pro HD/Developer/swish-devel/swish/lib/trace.pl:36:
Warning:    Loading module swish_trace abolished: [swish_trace:post_context/1]

Remedy ??

@jan

Great. As is, I don’t feel comfortable with this on a public server.

I like the wrapper solution more, but I have the impression it’s not safe because wrappers are not localized to the thread. I believe debug state is localized to a thread so in a sense, it would seem the debugger solution is safer. It’s a bit moot since the tracing feature itself is not permitted anyway.

[jan]

@ridgeworks

:- use_module(library(swish_trace, [swish_trace:post_context/1])).

You cannot import a non-exported predicate. You also do not want to import it, you want to define (some) rules for it. You do this (from any module) using

:- multifle swish_trace:post_context/1.

swish_trace:post_context(Dict) :-
   ...

No need for a use_module/1 of the library. The above creates a rule in the module. When the library is loaded it notices the predicate is multifile and it leaves the externally provided clauses alone. According to the ISO rules, any file using or defining clauses for a multifile predicate must declare it. ISO doesn’t talk about modules . Same, b.t.w. holds for your imports of library(sandbox). There is no need to do that. As in this case you do not explicitly import the multifile rules, nothing goes wrong. Not importing minimizes the footprint when your library is not used with Pengines.

@ridgeworks

I like the wrapper solution more, but I have the impression it’s not safe because wrappers are not localized to the thread.

That is true. Considering the complexity of the thing you want to wrap, I’d simply define a wrapper at the Prolog source level that is conditional on whether or not you want to trace the clpBNR execution. I doubt you’ll be able to measure the additional overhead. If that is an issue you can still use conditional compilation to allow for debugging or not. Unlike old versions, tail calls that pass all arguments are cheap these days, regardless of the number of arguments. Effectively, p(A,B,C.,,,,) :- q(A,B,C,...) is little more than a jump.

[ridgeworks]

@jan

You cannot import a non-exported predicate. You also do not want to import it, you want to define (some) rules for it.

Thanks for the reminder - I should have known based on the added definitions of prolog:message//1. So I’ll clean all that up in the next release.

In any case, all the machinery now appears to be working, but fixing my problem is a little trickier than I thought. I need an additional piece of information that can’t be easily passed in the attributes between the post_context call and the attribute_goals call. This is a bit difficult to implement when they execute in different threads. Basically, I need to distinguish between the case when answers are being produced (abbreviated version) and the general case required by a call to copy_term (“kitchen sink” version). So more thought required.

[ridgeworks]

Got stuck so opened a new topic:

Use of `attribute_goals//1` to compose answers

Apologies on the length of this post but think I need to provide some motivation. Background: Attributed variables are a core feature supporting many (most?) CLP dialects on Prolog. By permitting logic variables to be “annotated”, they effectively permit constraint networks to be constructed without requiring a separately managed constraint store, really a brilliant idea IMHO. The constraint networks built using attributed variables are terms which are typically cyclic (they’re graphs) and can…