Skip to content

Various improvements to provisional.py #387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Various improvements to provisional.py #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
137c650
Various improvements to provisional.py
aeliasso Sep 19, 2025
d7233ab
Add pyproject.toml with line length for Black
aeliasso Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 44 additions & 18 deletions py/dml/provisional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,30 @@
from . import logging
from . import messages


class ProvisionalFeature(abc.ABC):
def tag(self) -> str:
return self.__class__.__name__

@abc.abstractproperty
def __doc__(self): pass
@property
@abc.abstractmethod
def __doc__(self):
pass

@abc.abstractproperty
def short(self) -> str: pass
@property
@abc.abstractmethod
def short(self) -> str:
pass

@abc.abstractproperty
def stable(self) -> str: pass
@property
@abc.abstractmethod
def stable(self) -> bool:
pass

# Whether the feature is included in 1.2 documentation
dml12 = False


# tag -> feature
features: dict[str, ProvisionalFeature] = {}

Expand All @@ -34,7 +42,8 @@ def feature(cls: type[ProvisionalFeature]):

@feature
class explicit_param_decls(ProvisionalFeature):
'''<a id="explicit_param_decls"/>
"""<a id="explicit_param_decls"/>

This feature extends the DML syntax for parameter definitions to
distinguish between an intent to declare a new parameter, and an intent to
override an existing parameter (including when providing a definition
Expand Down Expand Up @@ -80,14 +89,21 @@ class explicit_param_decls(ProvisionalFeature):

Enabling the `explicit_param_decls` feature in a file only affects
the parameter definitions specified in that file.
'''
short = "Require := syntax for defining new params"
stable = True
"""

@property
def short(self) -> str:
return "Require := syntax for defining new params"

@property
def stable(self) -> bool:
return True
Copy link
Contributor

@lwaern-intel lwaern-intel Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a significant knee-jerk dislike of this. Why do this instead of stable = True, etc. -- what's the benefit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See microsoft/pyright#5564

As I understand it, it's how properties are supposed to be overridden. @abstractproperty is a deprecated way of defining a read-only property which has been replaced with the syntax: @property @abstractmethod. ( https://docs.python.org/3/library/abc.html#abc.abstractproperty )

Since it's a property, it needs to be overridden in subclasses. If assigned using = I think you are instead replacing the read-only property with an attribute which is read-write.

Copy link
Contributor

@lwaern-intel lwaern-intel Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From that thread:

I guess there's no way for an ABC to indicate that a class variable is required.

Buh. I really want to keep my stable = True syntax; if there is no go-to way to express mandatory class variables, we have to roll our own. I guess we could do that in feature. Something like:

def feature(cls: type[ProvisionalFeature]):
    assert issubclass(cls, ProvisionalFeature)
    for (attr, type) in (('__doc__', str), ('short', str), ('stable', bool)):
        assert hasattr(cls, attr) and isinstance(getattr(cls, attr), type), attr
    singleton = cls()
    features[singleton.tag()] = singleton
    return singleton

@mandolaerik Your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If assigned using = I think you are instead replacing the read-only property with an attribute which is read-write

No. Consider:

class X:
  foo = 1
class Y:
  @property
  def foo(self): return 1

In X, foo is available on class level, in Y the value is only available from the instance. You can say X.foo = 2 to change the class member, and you can shadow the class member in one specific instance by X().foo = 3. In Y, you can also change the class member, Y.foo = 4. Y().foo is indeed a read-only expression, in the sense that Y().__setattr__ rejects foo. However, the same is true for X if you seal the set of members through e.g. __slots__.

So, if you insist that properties are kept read-only on instance level, then use __slots__ or equivalent (e.g. dataclass). Or, if you want hard guarantees that thiings you intend as immutable stay immutable, then don't use Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get it now. You are only using these classes at the class level, not as instances. I didn't realize. In that case my change is indeed incorrect. Also in that case, I think the second part of the comment quoted is applicable:

It sounds like you're really looking for a way to specify a structural subtype. A Protocol may serve your purposes better than an ABC.

I have to read up on Protocol before I have an opinion on that. Assuming you want to keep the code as is, however, then I would like to turn the question around: Why do you declare these class members as properties in the abstract base class? To cause an error if a subclass forgets to override them, e.g. because the property is not printable?

Erik, in you example:

>>> class Y:
...   @property
...   def foo(self): return 1
...
>>> type(Y.foo)
<class 'property'>
>>> Y.foo=2
>>> type(Y.foo)
<class 'int'>

Y.foo is now an int and the original property is no more. It has ceased to exist. It is a late property. If this is the expected use of the abstract base class, then why declare a property in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point with Y.foo=2 is that python is hopelessly dynamic: even though the property is read-only in one sense, it's still possible to lift that protection. Nothing is truly protected. In particular, you might say:

class X:
  @property
  @abstractmethod
  def foo(self): pass

with the intention that overrides should be read-only as in:

class Y(X):
  @property
  def foo(self): return 1

but Python will happily swallow if someone says

class Z(X):
  foo = 2

Thus, a user of X does not have an immutability guarantee on foo.



@feature
class simics_util_vect(ProvisionalFeature):
'''<a id="simics_util_vect"/>
"""<a id="simics_util_vect"/>

This feature enables the `vect` type, based on the
`VECT` macro from the Simics C API (`simics/util/vect.h`).

Expand Down Expand Up @@ -130,18 +146,28 @@ class simics_util_vect(ProvisionalFeature):
When the `simics_util_vect` feature is disabled, usage of `vect` is an
error unless the [`experimental_vect` compatibility
feature](deprecations-auto.html#experimental_vect) is enabled.
'''
short = "Allow vect syntax based on the VECT macro"
stable = True
"""

@property
def short(self) -> str:
return "Allow vect syntax based on the VECT macro"

@property
def stable(self) -> bool:
return True

dml12 = True


def parse_provisional(
provs: list[("Site", str)]) -> dict[ProvisionalFeature, "Site"]:
provs: list[tuple[logging.Site, str]],
) -> dict[ProvisionalFeature, logging.Site]:
ret = {}
for (site, name) in provs:
for site, name in provs:
if name in features:
ret[features[name]] = site
else:
logging.report(messages.ENOPROV(
site, name, ', '.join(sorted(features))))
logging.report(
messages.ENOPROV(site, name, ", ".join(sorted(features)))
)
return ret
2 changes: 2 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[tool.black]
line-length = 79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not inherently opposed to this, but neither me nor Erik are using Black. You are free to use it, of course -- but enshrining it into the repo feels a bit much. The only one style guide DMLC is beholden to is Intel's own. I'd be OK with you putting pyproject.toml in the .gitignore though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about Black before. I'm open to try enforcing it for the DMLC repo, but it doesn't belong to this PR.

More specifically: Feel free to blackify all python files in the DMLC repo. The repo is small enough that we probably can sweep through the diff manually and see if it's a net win. If it is, then let's pioneer it by enforcing it with a test.