Misc

[JanWielemaker]

I did some reading through the latest version. Made a pull request with mostly minor issues. Running this version requires the git version of Prolog, so you may not want to merge this to the public master before the last release. The change that blocks this is a improvement to library(arithmetic) to allow for import/export of user defined functions. Your definition was rather dubious, defining the function in user, while the exported implementation may not get into user.

I removed the singleton style check and the singletons (the builtin editor makes this quick). Two singletons remain. See warnings. I think both are bugs (possibly not affecting global functioning of the library).

Please have a look. If you agree with this, I'm happy to add clpBNR to the public SWISH image.

[ridgeworks]

Thanks for the detailed PR - I'll process it and issue a new version. library(prolog_versions) in particular is quite useful; I just don't know how I would have found it.

I'm not sure about conditionally compiling to either toplevel or SWISH. In debugging on my local server, I've also used the server console toplevel for test purposes and I don't think I want to lose that. I agree there isn't much use for this in a production setting.

FWIW, there's at least one discontiguous predicate - narrowing_op/4 (ia_primitives.pl).

[JanWielemaker]

I just don't know how I would have found it.

Hard to find. Should probably be added to the main docs for that.

I'm not sure about conditionally compiling to either toplevel or SWISH.

Neither am I. Seems cleaner and you can debug the normal toplevel without SWISH.

FWIW, there's at least one discontiguous predicate - narrowing_op/4 (ia_primitives.pl).

I see. They are declared as discontiguous though, so you do not need to disable the global check.

[ridgeworks]

Hard to find. Should probably be added to the main docs for that.

In early testing I note the error message and a warning indicating the directive failed. However, it then proceeds to compile/load the rest of the module, which seems rather pointless and can potentially generate a lot of additional (generally pointless) error messages. Is there a way to just abandon the load when the directive fails?

Neither am I. Seems cleaner and you can debug the normal toplevel without SWISH.

I'll think about it. With all the different configs I have floating about, it's nice to use the same process for both to ensure consistency (same SWIP, same clpBNR).

[JanWielemaker]

Is there a way to just abandon the load when the directive fails?

As is, not. Ideally, loading the module should be abandoned and an exception should be propagated to load call. Abandoning loading a module is current not possible (except for calling halt/1). As you do it you also end up with a message and a succeeding load call. This is something that should be resolved at another level. For interactive use it doesn't matter too much. For scripts you can use --on-error=status to ensure that if the run somewhere prints an error message the process will exit with an error status.

[ridgeworks]

I've incorporated most of the changes in my local development version, with a couple of exceptions:

1. I have mixed feelings about the singleton check. While I recognize the benefits in the majority of situations, where I have multiple clauses with the same argument pattern it's distracting (to me) when the heads are different only because some of the clauses don't use one or more of the arguments. I've removed the global check and fixed most of the occurrences, but I disabled singleton checks in two places - builtin interval primitives (i.e., much of file ia_primitives.pl and the pd_f predicate (used in symbolic partial differentiation) in file ia_utilities.pl. I've satisified myself that the singletons in these two places are benign (by temporarily enabling the check) and I did fix the two bugs noted in your PR (one was benign and the other was a missed optimization opportunity).
2. I ported conditional compilation using temporary flag clpBNR_swish but only used it for adding functionality (portray ellipsis and answer formatting). I didn't use it to subtract functionality, at least for now, so I can still use the swish server top level console for local testing. I think this is appropriate because it leaves safety enforcement strictly up to sandbox policy rather than any application level configuration, but I don't feel very strongly about it.

I'm going to make the next clpBNR release dependent on the arithmetic function enhancement, presumably coming in the next SWIP development release. (I also need to look at the impact of this on one of my other packs.) I'll leave the PR open pending further comments.

[JanWielemaker]

1. where I have multiple clauses with the same argument pattern it's distracting (to me) when the heads are different only because some of the clauses don't use one or more of the arguments.

I typically "fix" that by using _Var in one and put a space in front of the Var in the other clause. That way they keep aligning while you get the immediate benefit to see that "ok, I'm not going to do anything with this variable".

2. but only used it for adding functionality (portray ellipsis and answer formatting). I didn't use it to subtract functionality

You would make me feel a bit safer by removing the user extension hook rather than defining the thing that is obviously not safe as safe and rely it cannot be used. I agree that disabling the normal toplevel integration is not that meaningful.

Next release may still be this year or early 2024.

[ridgeworks]

You would make me feel a bit safer by removing the user extension hook rather than defining the thing that is obviously not safe as safe and rely it cannot be used.

I don't really understand the safety concern. A user primitive can only be added to the clpBNR constraint network if it's defined in the clpBNR module namespace (see chk_primitive_/1). And it can only be invoked if it's in the constraint network (assuming that module local predicates are not accessible). So if the sandbox is doing its job, it's inherently safe and the only reason for declaring it so is to keep the sandbox safe_goal happy. Am I missing something?

Next release may still be this year or early 2024.

That was my expectation.

[ridgeworks]

I discovered that setting the optimise flag to true to improve arithmetic performance results in debugging/1 calls being "optimized" to fail. I then found the (undocumented?) optimise_debug flag which can be used to undo the debugging/1 optimization. However, unlike optimise, it is not scoped to the file.

My questions:

1. Should debugging/1 even be "optimized"? What are you actually saving?
2. Should the optimise_debug flag be scoped to the source file? Don't the same considerations apply to it as to the optimise flag?

[JanWielemaker]

1. Should debugging/1 even be "optimized"? What are you actually saving?

Yes. That is more or less the point of debug/3, debugging/1 and assertion/1. You can put them in your code for development and leave them there as annotation without paying a price. Note that debugging/1 is intended to be used as below, where the whole thing is removed under optimization.

    (    debugging(Topic)
    ->  <debug code>
    ;    true
    ).

2. Should the optimise_debug flag be scoped to the source file? Don't the same considerations apply to it as to the optimise flag?

I don't know. I'm not too happy with the file scoping of flags in general. Defining a flag to be scoped is rather arbitrary. Possibly we should have something like push_prolog_flag/2 or local_prolog_flag/2 or some other way to make it easier for users to change flags temporarily, both in source files and at runtime? As is, scoped Prolog flags are defined in init.pl and debug is a library. Seems a poor idea to stretch this. Another solution might be to allow libraries to define that their (new) Prolog flags are file scoped.

[ridgeworks]

Note that debugging/1 is intended to be used as below, where the whole thing is removed under optimization.

This is definitely not my use case; I need more dynamic control (not just load time). At the very least I need to decouple it from the optimise flag, i.e., optimise = true and optimise_debug = false.

I don't know. I'm not too happy with the file scoping of flags in general.

But subject to a general solution, specifically what about the optimise_debug flag? How is it any different that the optimise flag? Indeed its default value is the value of the optimise flag (the root of my problem?). So what's the harm in scoping it to the file like the optimise flag? Indeed, it seems to fit your use case perfectly.

I do agree the whole issue of flags is a bit of a nightmare, particularly in a multi-threaded environment, nbut this issue is much more contained.

[ridgeworks]

This is definitely not my use case; I need more dynamic control (not just load time).

In hindsight, this was probably why I was using debugging/2 - it wasn't susceptible to optimization.

[JanWielemaker]

In hindsight, this was probably why I was using debugging/2 - it wasn't susceptible to optimization.

That should have been considered a bug except that debugging/2 is documented for (IDE) tools rather than for debugging code. For now, the quickest way around it to change and restore the optimize_debug flag. A more permanent solution is desirable, but I do not like adding more "wrong" code. This is a rather borderline use case for the debug API.

[ridgeworks]

For now, the quickest way around it to change and restore the optimize_debug flag.

Will do.

IMO, I think the original bug was to couple arithmetic and debug optimization through the optimise flag. All the (load time) optimizations (artithmetic, debug, unify, clpfd, ...) should really be independent and have the same scope, however that's defined.

[JanWielemaker]

IMO, I think the original bug was to couple arithmetic and debug optimization through the optimise flag. All the (load time) optimizations (artithmetic, debug, unify, clpfd, ...) should really be independent and have the same scope, however that's defined.

Probably. It needs some thinking though. Some of these flags are part of the system and affect the core of the system. Others are managed by libraries. We cannot have the system acting on flags that are controlled by and affect libraries.

[ridgeworks]

I've pushed a new release (0.11.5) of clpBNR which incorporates most of your suggestions and a number of additional enhancements and fixes. In particular I found a simple and effective solution to my answer formatting problem - on expand_answer/2 or post_context/2 (the latter just invokes the former with the bindings in the dict.) just "disconnect" the answer variables from the constraint network when in non-verbose mode using a setarg. If query is non-deterministic, on backtracking from the toplevel, they get reconnected and the next answer is generated. All the relevant info is captured in the attribute; the threading structure doesn't matter, so I think it fits with the SWIP design intent.

Other than using swish_debug, there is now very little swish specific code:

:- if(current_prolog_flag(clpBNR_swish, true)).

% portray (HTML)
:- use_module(library(http/html_write)).
:- multifile(term_html:portray//2).

term_html:portray('$clpBNR...'(Out),_) -->   % avoid quotes on stringified number in ellipsis format
    { string(Out) },
    html(span(class('pl-float'), [Out, '...'])).

% for answer formatting
:- multifile(swish_trace:post_context/1).

swish_trace:post_context(Dict) :-
    expand_answer(Dict.bindings,_).

:- endif.  % current_prolog_flag(clpBNR_swish, true)

Tested on my local swish copy with SWIP 9.1.22 and all seems good to me.

Other than the issues identified earlier in this discussion, your suggested changes should be included in 0.11.5 so I'm going to close this PR without merge. Feel free to open a new one if you think further changes are warranted.