Skip to content

revert some changes to TypedDict which crept in via Flux#449

Merged
mtitov merged 2 commits intodevelfrom
revert/tdict
Oct 31, 2025
Merged

revert some changes to TypedDict which crept in via Flux#449
mtitov merged 2 commits intodevelfrom
revert/tdict

Conversation

@andre-merzky
Copy link
Member

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @andre-merzky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses and reverts specific modifications that were previously introduced into the TypedDict implementation, likely as an unintended consequence of a prior merge or integration. The core objective is to refine the internal mechanisms for data initialization and assignment within the TypedDict class, aiming for more predictable and robust behavior by simplifying conditional logic and ensuring the consistent presence of the internal data store.

Highlights

  • Simplified Initialization: The __init__ method has been refactored to consistently use self.update() for initializing the internal _data dictionary with default values and any provided from_dict arguments, removing previous redundant assignments.
  • Unified Setter Logic: The __setitem__ and __setattr__ methods no longer conditionally check the _check flag before calling _verify_setter. This ensures that the _verify_setter method is always applied for both item and attribute assignments, streamlining the data validation process.
  • Lazy Data Initialization: An improvement has been made to the __getattr__ method to lazily initialize the internal _data dictionary if it has not yet been set. This prevents potential AttributeErrors when _data is accessed via attribute lookup before explicit initialization.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.21%. Comparing base (ba00900) to head (3aec843).
⚠️ Report is 3 commits behind head on devel.

Files with missing lines Patch % Lines
src/radical/utils/typeddict.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            devel     #449      +/-   ##
==========================================
+ Coverage   61.19%   61.21%   +0.01%     
==========================================
  Files          66       66              
  Lines        7363     7359       -4     
==========================================
- Hits         4506     4505       -1     
+ Misses       2857     2854       -3     

☔ 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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the TypedDict class initialization and setter methods to simplify the code and fix initialization order issues. The changes ensure that _data is properly initialized lazily before any update operations occur.

  • Simplified __init__ method by consistently using update() for all initialization paths
  • Removed redundant _check conditionals in setter methods by always calling _verify_setter
  • Added lazy initialization of _data in __getattr__ to prevent initialization order issues

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


if from_dict:
self.update(from_dict)
self.update(from_dict)
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

Calling self.update(from_dict) when from_dict is None will cause an AttributeError since None does not have an items() method. While the update() method checks if not other:, this will only skip None and empty dicts. However, the call is unconditional here. Consider adding a conditional check before calling update(from_dict), similar to how kwargs is handled on line 167.

Suggested change
self.update(from_dict)
if from_dict is not None:
self.update(from_dict)

Copilot uses AI. Check for mistakes.
self._data[k] = self._verify_setter(k, v)
else:
self._data[k] = v
self._data[k] = self._verify_setter(k, v)
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

[nitpick] The _verify_setter method at line 461 returns v unchanged when _check is False or when there is no _schema. By removing the if self._check: conditional here and always calling _verify_setter, the behavior is preserved for cases where _check is True. However, this now adds an extra function call overhead even when _check is False. While functionally correct, this could be a minor performance regression for the common case where _check is False (the default). Consider whether this performance tradeoff is acceptable.

Suggested change
self._data[k] = self._verify_setter(k, v)
if self._check:
self._data[k] = self._verify_setter(k, v)
else:
self._data[k] = v

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request reverts some recent changes to TypedDict and introduces related cleanups. The changes are sound and improve the code's consistency and maintainability. Using self.update() for default values in __init__ ensures consistent processing, and the lazy initialization of _data in __getattr__ makes the implementation more robust. The removal of redundant checks in __setitem__ and __setattr__ is a good cleanup. I have reviewed the changes and found no issues.

Copy link
Collaborator

@mtitov mtitov left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!!

@mtitov mtitov enabled auto-merge October 31, 2025 18:36
@mtitov mtitov merged commit 5c86afb into devel Oct 31, 2025
9 checks passed
@mtitov mtitov deleted the revert/tdict branch October 31, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants