Skip to content

Revert lazy init of builder namespaces#7280

Merged
mbercx merged 1 commit into
mainfrom
revert-lazy-builder
Mar 12, 2026
Merged

Revert lazy init of builder namespaces#7280
mbercx merged 1 commit into
mainfrom
revert-lazy-builder

Conversation

@agoscinski
Copy link
Copy Markdown
Collaborator

@agoscinski agoscinski commented Mar 11, 2026

This reverts commit e68883e.

Due to the builder's usage in aiidalab we have to revert the change. A proper solution require breaking the API.

I link the original issue here:
#6994

@agoscinski agoscinski requested a review from khsrali March 11, 2026 06:35
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.86%. Comparing base (e398750) to head (66467d0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7280      +/-   ##
==========================================
+ Coverage   79.86%   79.86%   +0.01%     
==========================================
  Files         566      566              
  Lines       43905    43896       -9     
==========================================
- Hits        35060    35055       -5     
+ Misses       8845     8841       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Mar 12, 2026

Bugger, so an empty builder will contain empty dictionaries for monitor/stash/unstash/... ? What's the usage in AiiDAlab that requires reverting this commit?

@agoscinski agoscinski force-pushed the revert-lazy-builder branch from 5f33db0 to 66467d0 Compare March 12, 2026 08:35
@khsrali
Copy link
Copy Markdown
Collaborator

khsrali commented Mar 12, 2026

Bugger, so an empty builder will contain empty dictionaries for monitor/stash/unstash/... ? What's the usage in AiiDAlab that requires reverting this commit?

Yes, so some fixture broke.See here aiidalab/aiidalab-qe#1466
Because we were mainly concerned, because we don't know how many other packages are depending on the old behaviour of builder.. It's sort of a breaking change..

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Mar 12, 2026

Cheers, thanks @khsrali, I missed your ping on that PR. Let me have a look!

Clearly the change is breaking for some usage. We may indeed have to fix this in 3.0...

@khsrali
Copy link
Copy Markdown
Collaborator

khsrali commented Mar 12, 2026

@mbercx sure!
of course would be nice if you can find a solution for this!

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Mar 12, 2026

of course would be nice if you can find a solution for this!

I'm pretty passionate (aka OCD) about having clean builders, since I know the extra noise confuses users (what's this stash thing?). I guess I found my first task for the core days. ^^

@agoscinski
Copy link
Copy Markdown
Collaborator Author

Okay test passes on aiidalab qe now https://github.com/agoscinski/aiidalab-qe/actions/runs/22993313851/job/66759220294?pr=1

@agoscinski agoscinski enabled auto-merge (squash) March 12, 2026 08:49
@agoscinski agoscinski disabled auto-merge March 12, 2026 08:54
@mbercx
Copy link
Copy Markdown
Member

mbercx commented Mar 12, 2026

@agoscinski after our discussion with @edan-bainglass and @khsrali, I think it's ok to pull the trigger here and I'll open a separate PR to fix #6994 at the representation level.

@mbercx mbercx merged commit 6407800 into main Mar 12, 2026
34 checks passed
@mbercx mbercx deleted the revert-lazy-builder branch March 12, 2026 10:55
@khsrali
Copy link
Copy Markdown
Collaborator

khsrali commented Mar 12, 2026

I forgot I had a draft for the commit message. I'll out it here anyways :)

"""
This reverts commit e68883e.

The commit made PortNamespace children lazy (created on first access) instead of eager (all created upfront in __init__). Which was originaly to solve #6994
The complication: downstream code like aiidalab-qe was written against the eager behavior where builder._inputs() always returned a complete namespace tree. Therefor, this would have been a breaking change.
"""

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Mar 12, 2026

Thanks @khsrali! Sorry, adding that info to the commit message would have been sensible indeed, my bad. 🙈

@khsrali
Copy link
Copy Markdown
Collaborator

khsrali commented Mar 12, 2026

no worries all good :)

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants