Better Error Trapping in map #8307

GregoryTravis · 2023-11-15T14:08:58Z

Pull Request Description

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

This reverts commit c30d171.

JaroslavTulach

I am missing high-level overview in this PR. All I see is a bunch of xyz_primitive methods being added and I don't see a reason for that. The PR touching the core classes of the Enso engine. I'd like to understand its motivation before it gets in. I'd like to have a deeper review of the goals and ways we use to achieve them.

JaroslavTulach · 2023-12-06T04:38:46Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso

+    ## PRIVATE
+       Implementation of `map` that uses `vector_from_function_primitive`, which
+       does not do error wrapping, and does not have `on_problems`.
+    map_primitive : (Any -> Any) -> Vector Any


They are not 'private helpers' but just a 'developer oriented' variant of map that is more oriented towards internal implementations than the user-facing map whose error handling is more IDE-centric.

So I don't think they should necessarily be that hidden - they are not meant for the GUI users primarily, but library devs on outside libraries will definitely appreciate this function.

Is the above said by the same @radeusgd that usually takes rationalistic point of view? This is not a rationalistic approach at all. This stance is horribly pragmatic.

However we are talking about core API class of Enso. Where else shall we act rationalisticly than here? Moreover we are talking about builtin - e.g. part of the engine. Hence, I am willing to fight against debris like this to appear in these core classes.

Perhaps what we ultimately need is a user-facing Vector that mostly delegates to the internal Vector, but also implements things like Map_Error wrapping.

If you want internal array like object while implementing a library, use java.util.ArrayList or co. We haven't

Consolidate Vector and Array methods #6218

to introduce two types of vectors again.

prevent undocumented classes from being exposed to the user

Yes, please use private modules (e.g. real private, not just a comment). Should any problem (like #8469) appear, speak up.

JaroslavTulach · 2023-12-06T04:41:22Z

CHANGELOG.md

@@ -848,6 +849,7 @@
 [8105]: https://github.com/enso-org/enso/pull/8105
 [8150]: https://github.com/enso-org/enso/pull/8150
 [8235]: https://github.com/enso-org/enso/pull/8235
+[8307]: https://github.com/enso-org/enso/pull/8307


How comes that a PR like this doesn't have any overview at all?

No reference to any issue!? No summary? No explanation why chosen path is the right one?

JaroslavTulach · 2023-12-06T04:44:40Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

+       Implementation of `map` that does not do error wrapping, and does not
+       take an `on_problems` parameter.
+    map_primitive : (Any -> Any) -> Vector Any
+    map_primitive self function = Vector.map_primitive self function


What is a difference between map_primitive and map on_problems=Report_Error? I believe:

map_primitive self function = map self function Problem_Behavior.Report_Error

and I really don't understand why there should be two different methods to begin with!?

No, there is a difference.

Let me show by example:

f = if x == 3 then Error.throw (Illegal_Argument.Error "foo") else 100+x v = [1, 2, 3, 4, 5] r1 = v.map_primitive f r2 = v.map f Problem_Behavior.Report_Error

Now the results are following:

r1 = Error: Illegal_Argument.Error "foo" r2 = Error: Map_Error.Error 2 (Illegal_Argument.Error "foo")

The difference is that the user-facing map is wrapping the original error in a wrapper that allows our users to tell at which position in the collection the error appeared. This was a common issue that when the user was e.g. downloading 100 CSVs and the 87th download has failed - all they'd get is the failure but they would not know where it happened and the result of all other downloads was discarded.

Now with Report_Warning - all succeeding downloads are still kept, since the error value is replaced with Nothing so e.g. the user can decide to skip the problematic file.

And with Report_Error the first error is thrown, but the user has additional information on where it has happened, hopefully making it easier to analyze what went wrong.

The separate map_primitive variant is needed and cannot be private, because for 'lower-level' developers (e.g. not GUI end users but library devs), it often does not make sense to wrap the error in Map_Error - if the mapping operation is an internal implementation detail, e.g. checking a list of preconditions, I want the raw error to be thrown to the user, not a wrapped one - because I do not want to 'leak' the fact that I have used the map internally - it is not important to the user and in that case the Map_Error wrapping would only add noise.

And so this map_primitive functionality cannot be private, because we may very well want to use it in our other libraries; as well as even our more advanced users may want to use it in their internal code.

That is why we need both a Report_Error/Report_Warning modes for UX of our GUI users and a map_primitive mode which avoids these decorations and can retain the original APIs for lower level usage.

However, I don't think a special map_primitive function is the only solution to get this.

I think a viable alternative could be to instead allow a 4th mode on regular map - i.e. expose the map_primitive mode as vec.map f on_problems=Nothing or some other way (e.g. vec.map f on_problems=Report_Raw_Error) or whatever.

I think that approach is slightly cleaner as we still keep one map function.

OTOH the downside of this approach is slightly clashing requirements:

we want the on_problems to have a default value, and the default should be oriented towards our GUI users as they are our main focus - so it'd have to be Report_Error/Report_Warning.

that means that all usages in lib code would need to add on_problems=Nothing (or whatever syntax we choose) to the map. It seems slightly annoying and easy to forget to do so - so I imagine if we choose this, we will be forgetting from time to time and getting unnecessarily wrapped errors.

The advantage of map_primitive (I don't like the name) over the above solution is that it is e.g. much easier to find in our code all usages of it (search for map_primitive), versus the on_problems argument that is used on all kinds of functions so it may be harder to find.

r1 = Error: Illegal_Argument.Error "foo"
r2 = Error: Map_Error.Error 2 (Illegal_Argument.Error "foo")

Thank you for your explanation.

allow a 4th mode on regular map

Yes, that's a way better alternative!

advantage of map_primitive ... easy to forget to ... add on_problems=Nothing

Define an internal map_primitive extension method in each library and use it instead of map on_problems=Nothing where needed.

After discussion with @JaroslavTulach, I've moved the _primitive methods into extensions, and introduced a No_Wrap atom to implement them. Now all regular and primitive calls go through Array_Like_Helpers.vector_from_function.

Would we prefer perhaps map_no_wrap to map_primitive?

Personally, I like the map_no_wrap name :) well maybe map_without_wrapping? Idk which is better, but both seem better than map_primitive whose name is carrying some additional meaning that does not really fit the use-case.

JaroslavTulach · 2023-12-06T04:46:29Z

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso

@@ -1,7 +1,18 @@
 import project.Any.Any


This class shall be private as demonstrated by diff in #8469

hubertp · 2023-12-08T09:55:59Z

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso

+     Errors and Warnings that arise when executing the function are wrapped in
+     `Map_Error`.
+
+     Only `MAX_MAP_WARNINGS` warnings are attached to result values. After that,


Suggested change

Only `MAX_MAP_WARNINGS` warnings are attached to result values. After that,

Only `MAX_MAP_WARNINGS` number of warnings are attached to result values. After that,

hubertp

I thought that if we define the methods as extensions then no
from project.Data.Vector.Extensions import all is needed. Yet this PR seems to add it in numerous places. Is that still necessary?
I mixed extensions with conversions. Ignore, as per conversation.

JaroslavTulach

Please hide Data/Vector/Extensions.enso from Standard.Base APIs.

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso

JaroslavTulach · 2023-12-06T10:26:54Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

+       Implementation of `map` that does not do error wrapping, and does not
+       take an `on_problems` parameter.
+    map_primitive : (Any -> Any) -> Vector Any
+    map_primitive self function = Vector.map_primitive self function


r1 = Error: Illegal_Argument.Error "foo"
r2 = Error: Map_Error.Error 2 (Illegal_Argument.Error "foo")

Thank you for your explanation.

allow a 4th mode on regular map

Yes, that's a way better alternative!

advantage of map_primitive ... easy to forget to ... add on_problems=Nothing

Define an internal map_primitive extension method in each library and use it instead of map on_problems=Nothing where needed.

JaroslavTulach · 2023-12-08T13:53:32Z

distribution/lib/Standard/Table/0.0.0-dev/src/Extensions/Table_Ref.enso

@@ -1,6 +1,7 @@
 from Standard.Base import all
 import Standard.Base.Errors.Common.Index_Out_Of_Bounds
 import Standard.Base.Errors.Illegal_Argument.Illegal_Argument
+from Standard.Base.Data.Vector.Extensions import all


These extensions shouldn't be exported from Standard.Base they should be private for each project. Otherwise they become part of API (which is what I'd like to avoid). map_no_wrap shalln't be visible to Enso users.

They've been removed entirely.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector/Extensions.enso

GregoryTravis · 2023-12-08T21:39:25Z

After discussion with @JaroslavTulach and @hubertp, we agreed it would be best that the_primitive methods should not be in Standard.Base. While moving them and their tests to the Table and Database projects, it became clear that this is all overengineering, and it's better to get rid of the _primitive methods just pass the No_Wrap flag where necessary.

The methods have been removed.

GregoryTravis added 29 commits November 8, 2023 15:08

tests

2485d3e

Merge branch 'develop' into wip/gmt/8110-map-error

0cb3675

wip

dd0fdde

wip

ae1fd56

additional warnings

3cb9b4a

wip

506a13b

Merge branch 'develop' into wip/gmt/8110-map-error

f4427df

wip

b5d4325

cleanup

be40d60

nested wrapping

8205cac

multiple nestings

87c246b

wraps_error uses looks_for, test for should_fail_with

40ff235

wip

a66384a

Merge branch 'develop' into wip/gmt/8110-map-error

d1f7013

stack trace line fix

a4d9bbd

use catch_primitive internally

9e3f0f0

fix warning mapping, dtf spec

074ee0b

just one wrapper checker, vector spec

7a3800c

missing ctor, back to non-primitive catch

7516a6b

Merge branch 'develop' into wip/gmt/8110-map-error

f8f0b2f

back to c_p

b15c7a6

put old map back

059559b

wip

8409be6

unnest tests

85e3e2d

Array.map on_problems

b0e811a

wip

c30d171

Revert "wip"

178ccd4

This reverts commit c30d171.

better test names

fd3062c

warning logging

f31bc15

GregoryTravis changed the title ~~Wip/gmt/8110 map error~~ Better Error Trapping in map Nov 15, 2023

GregoryTravis linked an issue Dec 4, 2023 that may be closed by this pull request

Better Error Trapping in map #8110

Closed

GregoryTravis added 2 commits December 4, 2023 16:57

finish rename

d7804fa

Merge branch 'develop' into wip/gmt/8110-map-error

2754ef3

JaroslavTulach requested changes Dec 6, 2023

View reviewed changes

move to extensions

f619e44

enso-bot bot mentioned this pull request Dec 7, 2023

Inconsistent type reported by Meta for [] #8255

Closed

GregoryTravis added 2 commits December 7, 2023 10:45

Merge branch 'develop' into wip/gmt/8110-map-error

5c28d9e

combine Additional_Warnings error

97dc84c

GregoryTravis requested a review from JaroslavTulach December 7, 2023 16:27

GregoryTravis removed the CI: Ready to merge This PR is eligible for automatic merge label Dec 7, 2023

GregoryTravis mentioned this pull request Dec 7, 2023

Cannot make Array_Like_Helpers private #8469

Closed

GregoryTravis added 2 commits December 7, 2023 11:52

rename to map_no_wrap

6760cd0

do not catch and rethrow

0e7312e

hubertp approved these changes Dec 8, 2023

View reviewed changes

hubertp reviewed Dec 8, 2023

View reviewed changes

JaroslavTulach previously requested changes Dec 8, 2023

View reviewed changes

GregoryTravis added 4 commits December 8, 2023 09:07

Merge branch 'develop' into wip/gmt/8110-map-error

8e3e079

review

17c2eff

wip

561636c

remove _primitives entirely

6ec4cd6

Merge branch 'develop' into wip/gmt/8110-map-error

cad76a5

GregoryTravis requested a review from JaroslavTulach December 11, 2023 15:20

GregoryTravis added 2 commits December 11, 2023 16:16

remove unused should_fail_with function options

1465cf2

Merge branch 'develop' into wip/gmt/8110-map-error

c354de6

GregoryTravis added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Dec 12, 2023

remove expected_warning as function in Problems

ff469ff

GregoryTravis merged commit 1c815a3 into develop Dec 13, 2023
34 checks passed

GregoryTravis deleted the wip/gmt/8110-map-error branch December 13, 2023 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better Error Trapping in map #8307

Better Error Trapping in map #8307

GregoryTravis commented Nov 15, 2023 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Dec 6, 2023

JaroslavTulach Dec 6, 2023

JaroslavTulach Dec 6, 2023

radeusgd Dec 6, 2023

JaroslavTulach Dec 6, 2023

GregoryTravis Dec 6, 2023

radeusgd Dec 6, 2023

JaroslavTulach Dec 6, 2023

hubertp Dec 8, 2023

GregoryTravis Dec 8, 2023

hubertp left a comment •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Dec 6, 2023

JaroslavTulach Dec 8, 2023

GregoryTravis Dec 11, 2023

GregoryTravis commented Dec 8, 2023 •

edited

Loading

	Only `MAX_MAP_WARNINGS` warnings are attached to result values. After that,
	Only `MAX_MAP_WARNINGS` number of warnings are attached to result values. After that,

Better Error Trapping in map #8307

Better Error Trapping in map #8307

Conversation

GregoryTravis commented Nov 15, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach 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

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

hubertp left a comment • edited Loading

Choose a reason for hiding this comment

JaroslavTulach 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

GregoryTravis commented Dec 8, 2023 • edited Loading

GregoryTravis commented Nov 15, 2023 •

edited

Loading

hubertp left a comment •

edited

Loading

GregoryTravis commented Dec 8, 2023 •

edited

Loading