Skip to content

Conversation

@anton-ubi
Copy link
Contributor

@anton-ubi anton-ubi commented Nov 6, 2025

Summary.
Surface cleanup and typing annotation of the layer module.

Changes

  • remove spotted python-2 artifacts (objects, with_metaclass, super with args)
  • use super() when possible
  • use f-string when meaningful
  • add typing annotations when non ambiguous
  • explicit a bit more what parameters are actually hidden behind Layer object kwargs
  • Correct typo and grammar in docstrings and comments
  • Use Literal to annotate Layer types (Render, Util, Post)
  • Add Layer string and repr representation

Rational

  • Type annotations: The main goal is to enable better auto-completion when outlining layers. Prior to that PR, it is very difficult to know in advance the name and type of the parameters accepted by a Layer object.
  • Literal for LAYER_TYPES constant values: We can now benefit of the auto-completion to know which types exist as static type checkers now know about them
  • The rest is mostly house-cleaning to keep up, especially since we dropped python2.

Note to reviewers
PR can be reviewed per commit for smaller scoped diffs.
Those are only typing and syntax improvements. No change in behavior should occur.

@lithorus
Copy link
Collaborator

lithorus commented Nov 17, 2025

Thank you for the PR. Really nice to have proper completion when building outlines.
I noticed that you moved the values from constants.py into layer.py, but still leaving them in constants.py. They should only be in 1 place. (and perhaps also update the PR description about the move)

Otherwise it looks good to me.

@anton-ubi
Copy link
Contributor Author

Hey there! Thanks for taking the time to look at that pretty big chunk of diffs.

Regarding the layer types, if you're referring to the line _LayerT = Literal["Render", "Util", "Post"], please note that it isn’t meant to replace the values defined historically in constants.py. The list in constants.py remains the single source of truth and stays exactly as it is.

The Literal[...] alias is only there to support static typing and IDE auto-completion. It has no impact at runtime and doesn’t override or duplicate the actual constants.

That said, it makes sense to keep the annotation type alongside the existing constants, so I’ll move the type alias there.

One thing to flag, though: the name LayerType is already used in the layer module as the metaclass for the Layer class. That creates an ambiguity between the metaclass and the string-based annotation type. I suggest renaming the annotation alias to something shorter, such as LayerT to prevent confusion.

I'll address the changes. Let me know.

@lithorus
Copy link
Collaborator

Not 100% optimal, but better to have them all in the same place at least.

For compatibility reasons it will have to be like this until we depricate constants.LAYER_TYPES and the benefits of proper completion is worth it IMHO.

Copy link
Collaborator

@lithorus lithorus left a comment

Choose a reason for hiding this comment

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

I will approve it, but would prefer that @ramonfigueiredo or @DiegoTavares also have a quick look at it if possible before merging.

@anton-ubi
Copy link
Contributor Author

Hey @lithorus ,

in that case, may I suggest to get back to my original intention that was to move to a string enum ?

Here's the initial diff: 045903d

I moved away from that proposal as I felt it was getting a bit too intrusive and not related to cosmetics and completion changes I wanted this PR to be about.
But since deprecating the current construct does not seem to bother you, I do think StrEnum would be a solid and better contender to replace it.

@lithorus
Copy link
Collaborator

Hey @lithorus ,

in that case, may I suggest to get back to my original intention that was to move to a string enum ?

Here's the initial diff: 045903d

I moved away from that proposal as I felt it was getting a bit too intrusive and not related to cosmetics and completion changes I wanted this PR to be about.
But since deprecating the current construct does not seem to bother you, I do think StrEnum would be a solid and better contender to replace it.

Yeah, I liked that one much better when reviewing the commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants