[FIX] Fixes for deprecations in 3.26, and for changed behaviour of file dialog #4643

janezd · 2020-04-10T13:35:14Z

Issue

DomainContextHandler: Remove support for Variables stored as strings:
We promised to drop support for storing variables as string settings in 3.26. To keep the promise, we booby-trapped tests.

Tests for OWFile: orangewidgets.utils.filedialogs.open_filename_dialog changed behaviour in biolab/orange-widget-base#53: it will now return the correct file extension and reader when the user finds a file through the "All formats" filter. Before, it returned None and there was a test that expected it. Because of this test_future_compatibility fails.

Until Orange-widget-base is released, the test can't work for released version and for master.

Also fixes #4613.

Description of changes

Context handling:

Remove support for variables stored as strings.
Fixes the two non-compliant widgets: OWCorrelations and OWSelectColumns (commit is cherry-picked from Select Columns: Store settings as Variable #4616).
Raise an exception if Variable is stored as a non-context setting. This is not directly related to this PR, but prudent. It may cause some new errors, but these errors are better raised in this place then further down the path.
Minor fix: replace 1 with ContextHandler.MATCH, which is now provided in widget-base.

Lint fails because it tries to check file Orange/widgets/data/contexthandlers.py, which no longer exists. This is unavoidable (unless we improve lint configuration, but this is an unrelated matter for another PR). This will happen only on this PR, which removes the file.

Test for file:

Test is commented out, with a notice to enable it when the time comes.

Includes

Code changes
Tests

janezd · 2020-04-10T14:26:48Z

.

janezd · 2020-04-12T10:53:25Z

Orange/widgets/settings.py

-            attr_type -= 100
-        else:
-            cls._warn_about_str_var_settings(setting)
+        attr_type %= 100


While attr_type will now always be > 100, let's play it safe for any possible unmigrated settings.

Ah, this is the compatibility trick that allows for newer settings to work with older versions. Could you please this comment to the code? There should probably also be a test that confirms old and new values working.

Good idea.
Maybe a comment in the code (not just in the PR) would help future generations decipher why attr_type is being percentaged :)

You're right it's cryptic. I added a comment.

Ah, this is the compatibility trick that allows for newer settings to work with older versions. Could you please this comment to the code? There should probably also be a test that confirms old and new values working.

I added a comment, but not a test. New settings won't work with older versions (I guess), the trick might help in other direction, but I wouldn't guarantee it with a test. :)

janezd · 2020-04-15T22:10:10Z

Would somebody please review this on Thursday, otherwise it will be a blocker on Friday? @lanzagar, @markotoplak?

lanzagar · 2020-04-16T07:45:20Z

Orange/widgets/settings.py

-            attr_type -= 100
-        else:
-            cls._warn_about_str_var_settings(setting)
+        attr_type %= 100


Good idea.
Maybe a comment in the code (not just in the PR) would help future generations decipher why attr_type is being percentaged :)

lanzagar · 2020-04-16T07:59:58Z

Orange/widgets/settings.py

+            else:
+                raise ValueError("Variables must be stored as ContextSettings; "
+                                 f"change {setting.name} to ContextSetting.")


Seems ok to me, but I have no idea about all the types of value we support and need to handle here.
And because this looks like it is not just removing a deprecation, but changing functionality - somebody please confirm this is intended and OK.

If it was a ContextSetting and str it gave a deprecation before, now it goes to the below default return copy...
What happens in that case? After removing deprecation warnings I would expect old code to get some error that still makes it clear that things changed and this is not supported anymore.

If it was a Variable and not a ContextSetting before it went to the below default return copy.... After this it will raise the new ValueError. Is this a fix for something bundled with the removal of the deprecation?

Yes, there are two options: when we removing support for something we can either raise an exception or change the behaviour. In this case, it changes the behaviour that, in my opinion, should be changed because it was ugly and dangerous. Plus, raising an exception doesn't work correctly.

Before, it worked like this. You had a context setting and it stored a string "foo". The handler didn't know if foo is a name of a variable or just a string, so it wouldn't know how to save it. The only thing it could do was to check whether there existed a variable named foo and if it did, it assumed the setting "foo" referred to that variable. So it encoded the string as a (variable name, variable type) pair. (And, for the last few years, also gave a warning.) This was obviously not foolproof -- there could exist a variable whose name would coincidentally match an unrelated string setting. But even if this happened, the widget wouldn't notice since the variable was decoded back to string "foo". (The only side effect would be that a context without variable "foo" wouldn't match.)

With the current version, that is, coding as a string, domain context matching (in incorrectly implemented widgets) would cease to observe this variable, possibly resulting in an error due to mismatched context. While errors should generally be detected sooner (=here) than later (=some time after openContext), I'd in this case prefer later.

Why? If we change the code so that it would raise an exception, we would get an exception whenever a name of some variable coincidentally matched a string setting (and these kinds of errors did not happen before this PR!). That is, we would in essence have "invalid variable names" -- if there is a string setting "foo", the widget would fail it the domain contained a variable "foo".

In short, the only benefit of raising an exception here is to raise it at the point of the problem and not later in the code. The downside is that detection of a problem has false positive and it puts arbitrary restraints on variable names, that result in raising strange exception to the end-users.

No, this is unrelated, it's just something I discovered when simplifying the conditions. If a Variable was stored in a non-context setting, it would be encoded as a value and pickled. After loading a schema, we would probably get an error because the widget's setting would contain a reference to a variable that does not appear in the domain. So it is better to raise an exception when saving the schema. This is thus not really a new exception, just an exception that can be (reliable) shown earlier.

lanzagar · 2020-04-16T08:05:09Z

Orange/widgets/data/owcorrelations.py

@@ -415,7 +415,7 @@ def commit(self):

        # data has been imputed; send original attributes
        self.Outputs.features.send(AttributeList(
-            [self.data.domain[name] for name, _ in self.selection]))
+            [self.data.domain[var.name] for var in self.selection]))


self.data.domain[var.name] is a bit redundant, domain can be indexed with variables as well as names:
self.data.domain[var]

This should work even if var is loaded from settings and is not the same object right? Should have the same hash even in those exotic cases with compute_values etc? If not sure you can always ignore my comment above and leave it as it is now :)

I thought so, too, but it doesn't work. compute_value is a part of a hash (and has to be, otherwise domain transformation would mistook two variables to be equivalent when they are not).

We have to use var.name here.

lanzagar · 2020-04-16T08:12:52Z

Orange/widgets/data/owcorrelations.py

+            context.values["selection"] = [(var.name, vartype(var))
+                                           for var in sel[0]]
+        if version < 3:
+            sel = context.values["selection"]
+            context.values["selection"] = ([(name, vtype + 100)
+                                            for name, vtype in sel], -3)


I am confused, isn't selection now supposed to be a list of variables?

The widget's attribute (as in widget.selection) is a list of Variables. Context handler encodes it (as in context.values["selection"]) as a list of (name, type) pairs.

In settings version 2 the widget had an attribute selection which contains a list of (name, type) pairs. This was safe for saving, but does the encoding within the widget instead of in the handler. The handler thus saw strings with names of variables and issued a warning.

In settings version 3, the widget's selection is a list of Variables, and encoding is done within the handler.

Coincidentally, the encoded setting looks (almost) the same, because the widget's encoding was (almost) the same as encoding by the handler. So migration just adds 100 to vartypes and appends -3 to the tuple to mark it as a list.

janezd

It appears that the only change I'd make is adding a comment. Some of my answers are long. You can call me on Slack if you want to discuss it live.

markotoplak · 2020-04-16T07:29:31Z

Orange/widgets/settings.py

-            attr_type -= 100
-        else:
-            cls._warn_about_str_var_settings(setting)
+        attr_type %= 100


Ah, this is the compatibility trick that allows for newer settings to work with older versions. Could you please this comment to the code? There should probably also be a test that confirms old and new values working.

OWCorrelations: Store selection setting as [Variable], not [(str, int)]

ca11cc1

janezd mentioned this pull request Apr 10, 2020

Select Columns: Store settings as Variable #4616

Closed

2 tasks

janezd changed the title ~~DomainContextHandler: Remove support for Variables stored as strings~~ [FIX] Fixes for deprecations in 3.26, and for changed behaviour of file dialog Apr 10, 2020

janezd commented Apr 12, 2020

View reviewed changes

lanzagar reviewed Apr 16, 2020

View reviewed changes

janezd and others added 3 commits April 16, 2020 11:48

DomainContextHandler: Remove support for variables stored as strings

d4c1666

Store settings as Variable

4a0cd77

OWFile: Comment out a failed test that currently can't be fixed

d89e896

janezd force-pushed the remove-str-settings branch from 128af91 to d89e896 Compare April 16, 2020 09:48

janezd commented Apr 16, 2020

View reviewed changes

markotoplak approved these changes Apr 16, 2020

View reviewed changes

lanzagar merged commit c8ab897 into biolab:master Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Fixes for deprecations in 3.26, and for changed behaviour of file dialog #4643

[FIX] Fixes for deprecations in 3.26, and for changed behaviour of file dialog #4643

janezd commented Apr 10, 2020 •

edited

Loading

janezd commented Apr 10, 2020 •

edited

Loading

janezd Apr 12, 2020

markotoplak Apr 16, 2020

lanzagar Apr 16, 2020

janezd Apr 16, 2020 •

edited

Loading

janezd Apr 16, 2020

janezd commented Apr 15, 2020

lanzagar Apr 16, 2020

lanzagar Apr 16, 2020

janezd Apr 16, 2020

lanzagar Apr 16, 2020

lanzagar Apr 16, 2020

janezd Apr 16, 2020 •

edited

Loading

lanzagar Apr 16, 2020

janezd Apr 16, 2020

janezd left a comment •

edited

Loading

markotoplak Apr 16, 2020

[FIX] Fixes for deprecations in 3.26, and for changed behaviour of file dialog #4643

[FIX] Fixes for deprecations in 3.26, and for changed behaviour of file dialog #4643

Conversation

janezd commented Apr 10, 2020 • edited Loading

Issue

Description of changes

Includes

janezd commented Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janezd Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janezd commented Apr 15, 2020

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

janezd Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janezd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janezd commented Apr 10, 2020 •

edited

Loading

janezd commented Apr 10, 2020 •

edited

Loading

janezd Apr 16, 2020 •

edited

Loading

janezd Apr 16, 2020 •

edited

Loading

janezd left a comment •

edited

Loading