Fix: table delete callback error in Taipy GUI #2768

ibrahim307le · 2025-11-13T08:11:59Z

What type of PR is this? (Check all that apply)

Refactor
Feature
Bug Fix
Optimization
Documentation Update

Description
Provide a clear and concise description of the changes introduced in this PR.

Related Tickets & Documents

For pull requests that relate to or close an issue, please include them below.
Following [GitHub's guidance on linking issues] (https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) helps with automation.

Example:

Closes docs: convert markdown templates to issue forms #1234
Related to #5678
Related Issue
Closes
How to reproduce the issue
Provide step-by-step instructions to reproduce the issue or test the feature.
Backporting
Specify any branches or releases this change needs to be backported to.
This change should be backported to:
3.0
3.1
4.0
develop

Checklist
We encourage keeping the code coverage percentage at 80% or above.

This solution meets the acceptance criteria of the related issue.
The related issue checklist is completed.
This PR includes unit tests for the developed code.
If not, explain why:
End-to-End tests have been added or updated.
If not, explain why:
The documentation has been updated, or a dedicated issue has been created.
If not, explain why:
The release notes have been updated.
If not, explain why:

FabienLelaquais · 2025-11-14T09:09:35Z

@ibrahim307le Thanks a lot for your input.
The issue actually has deeper implications in the overall product, so we need some time to clearly understand what the impacts of your changes are (beside the fact that test should be well, in the tests directory).
Forgive the little delay that we need to come back to you, but I promise we will.

ibrahim307le · 2025-11-16T14:49:05Z

Thanks for this opportunity, and I look forward to updating the changes needed for this.

FredLL-Avaiga

Every data access has to be made through the data accessors.
Gui should have no direct dependency to pandas
I don't see any link with the referenced issue in the description.
If there's no issue connected this PR will be rejected

Copilot

Pull request overview

This PR attempts to fix a table delete callback error in Taipy GUI by introducing new helper methods for variable access and modifying the table_on_delete and table_on_add implementations. However, the implementation has several critical issues that break existing functionality.

Key Changes:

Added _get_var_from_name() and _set_var_in_name() helper methods for variable retrieval and storage
Replaced the data accessor pattern in table_on_delete() and table_on_add() with direct pandas DataFrame manipulation
Added pandas import at module level
Added a basic test for table_on_delete()

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.

File	Description
taipy/gui/gui.py	Introduced helper methods for variable access and modified table callback implementations, breaking the existing data accessor architecture
taipy/gui/test_table.py	Added a test that checks internal implementation details rather than actual functionality

Comments suppressed due to low confidence (1)

taipy/gui/gui.py:1969

Variable var is not used.

            var = self._get_var_from_name(var_name)

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

Copilot · 2025-11-24T15:49:41Z

taipy/gui/gui.py

+        """Set or update a variable in internal store, GUI state, or globals."""
+        try:
+            if not hasattr(self, "_variables"):
+                self._variables = {}
+
+            self._variables[var_name] = value
+
+            if hasattr(self, "state") and var_name in getattr(self, "state", {}):
+                self.state[var_name] = value
+                return
+
+            if hasattr(self, var_name):
+                setattr(self, var_name, value)
+                return
+
+            globals()[var_name] = value
+
+        except Exception as e:
+            _warn(f"Failed to set variable '{var_name}'.", e)
+


The logic in _set_var_in_name always sets the value in self._variables (line 1892) AND then tries to set it elsewhere. This creates data inconsistency issues. If a variable exists in the state, it will be stored in both self._variables and self.state, potentially leading to synchronization issues. The method should have a clear priority order and only store in one location.

Suggested change

"""Set or update a variable in internal store, GUI state, or globals."""

try:

if not hasattr(self, "_variables"):

self._variables = {}

self._variables[var_name] = value

if hasattr(self, "state") and var_name in getattr(self, "state", {}):

self.state[var_name] = value

return

if hasattr(self, var_name):

setattr(self, var_name, value)

return

globals()[var_name] = value

except Exception as e:

_warn(f"Failed to set variable '{var_name}'.", e)

"""Set or update a variable in the highest-priority location: state, attribute, or internal store."""

try:

if not hasattr(self, "_variables"):

self._variables = {}

# Priority 1: state

if hasattr(self, "state") and var_name in getattr(self, "state", {}):

self.state[var_name] = value

return

# Priority 2: attribute

if hasattr(self, var_name):

setattr(self, var_name, value)

return

# Priority 3: internal store

self._variables[var_name] = value

except Exception as e:

_warn(f"Failed to set variable '{var_name}'.", e)

Copilot · 2025-11-24T15:49:41Z

taipy/gui/gui.py

        """
        try:
-            setattr(state, var_name, self._get_accessor().on_delete(getattr(state, var_name), payload))
+            var = self._get_var_from_name(var_name)


The variable var is assigned but never used. This line (1969) duplicates the assignment on line 1970 and should be removed.

Suggested change

var = self._get_var_from_name(var_name)

Copilot · 2025-11-24T15:49:42Z

taipy/gui/gui.py

+        #This function sets the value of a specific cell in the tabular dataset stored in
           *var_name*, typically bound to the *data* property of a table control.


The comment formatting is broken. The # should be indented properly on its own line with triple quotes, not inline. This should be formatted as:

"""Default implementation of the `on_edit` callback for tables. This function sets the value of a specific cell in the tabular dataset stored in *var_name*, typically bound to the *data* property of a table control.

Suggested change

#This function sets the value of a specific cell in the tabular dataset stored in

*var_name*, typically bound to the *data* property of a table control.

This function sets the value of a specific cell in the tabular dataset stored in

*var_name*, typically bound to the *data* property of a table control.

Copilot · 2025-11-24T15:49:42Z

taipy/gui/gui.py

+    def _get_var_from_name(self, var_name: str):
+        """Retrieve a variable from internal store, GUI state, or globals."""
+        try:
+
+            if not hasattr(self, "_variables"):
+                self._variables = {}
+
+            if var_name in self._variables:
+                return self._variables[var_name]
+
+            if hasattr(self, "state") and var_name in getattr(self, "state", {}):
+                return self.state[var_name]
+
+            if hasattr(self, var_name):
+                return getattr(self, var_name)
+
+            if var_name in globals():
+                return globals()[var_name]
+
+        except Exception as e:
+            _warn(f"Error while retrieving variable '{var_name}'.", e)
+            return None
+
+        _warn(f"Variable '{var_name}' not found.")
+        return None
+
+    def _set_var_in_name(self, var_name: str, value):
+        """Set or update a variable in internal store, GUI state, or globals."""
+        try:
+            if not hasattr(self, "_variables"):
+                self._variables = {}
+
+            self._variables[var_name] = value
+
+            if hasattr(self, "state") and var_name in getattr(self, "state", {}):
+                self.state[var_name] = value
+                return
+
+            if hasattr(self, var_name):
+                setattr(self, var_name, value)
+                return
+
+            globals()[var_name] = value
+
+        except Exception as e:
+            _warn(f"Failed to set variable '{var_name}'.", e)
+


The _get_var_from_name and _set_var_in_name methods completely ignore the state parameter that is passed to table_on_delete and table_on_add. These methods should use state (via getattr(state, var_name) and setattr(state, var_name, value)) instead of implementing a custom variable lookup/storage mechanism. The State object is the proper way to access and modify variables in Taipy GUI callbacks.

Suggested change

def _get_var_from_name(self, var_name: str):

"""Retrieve a variable from internal store, GUI state, or globals."""

try:

if not hasattr(self, "_variables"):

self._variables = {}

if var_name in self._variables:

return self._variables[var_name]

if hasattr(self, "state") and var_name in getattr(self, "state", {}):

return self.state[var_name]

if hasattr(self, var_name):

return getattr(self, var_name)

if var_name in globals():

return globals()[var_name]

except Exception as e:

_warn(f"Error while retrieving variable '{var_name}'.", e)

return None

_warn(f"Variable '{var_name}' not found.")

return None

def _set_var_in_name(self, var_name: str, value):

"""Set or update a variable in internal store, GUI state, or globals."""

try:

if not hasattr(self, "_variables"):

self._variables = {}

self._variables[var_name] = value

if hasattr(self, "state") and var_name in getattr(self, "state", {}):

self.state[var_name] = value

return

if hasattr(self, var_name):

setattr(self, var_name, value)

return

globals()[var_name] = value

except Exception as e:

_warn(f"Failed to set variable '{var_name}'.", e)

def _get_var_from_name(self, state: State, var_name: str):

"""Retrieve a variable from the provided State object."""

try:

return getattr(state, var_name, None)

except Exception as e:

_warn(f"Error while retrieving variable '{var_name}' from state.", e)

return None

_warn(f"Variable '{var_name}' not found in state.")

return None

def _set_var_in_name(self, state: State, var_name: str, value):

"""Set or update a variable in the provided State object."""

try:

setattr(state, var_name, value)

except Exception as e:

_warn(f"Failed to set variable '{var_name}' in state.", e)

Copilot · 2025-11-24T15:49:42Z

taipy/gui/gui.py

+                setattr(self, var_name, value)
+                return
+
+            globals()[var_name] = value


Modifying global variables directly (line 1902) is a dangerous practice that can lead to unpredictable behavior and side effects. This violates best practices for state management in Python applications. Variables should be managed through the proper State object mechanism rather than manipulating globals.

Copilot · 2025-11-24T15:49:43Z

taipy/gui/gui.py

 import json
 import math
 import os
+import pandas as pd


The pandas import is added at the module level but the code should handle cases where pandas might not be available. The existing codebase uses a data accessor pattern that abstracts away pandas-specific operations. Adding a direct pandas dependency here could cause issues if pandas is not installed or in environments where it's optional.

Suggested change

import pandas as pd

Copilot · 2025-11-24T15:49:43Z

taipy/gui/gui.py

+            var = self._get_var_from_name(var_name)
+            df = self._get_var_from_name(var_name)
+            if isinstance(df, pd.DataFrame):
+                idx = payload.get("index")
+                if idx is None:
+                    _warn("No index provided for deletion.")
+                    return
+                try:
+                    df = df.drop(df.index[int(idx)])
+                    df.reset_index(drop=True, inplace=True)
+                    self._set_var_in_name(var_name, df)
+                except (ValueError, IndexError, KeyError):
+                    if idx in df.index:
+                        df = df.drop(idx)
+                        df.reset_index(drop=True, inplace=True)
+                        self._set_var_in_name(var_name, df)
+                    else:
+                        _warn(f"Row '{idx}' not found in DataFrame.")
+            else:
+                _warn(f"Variable '{var_name}' is not a DataFrame.")


The new implementation bypasses the existing data accessor architecture (self._get_accessor()) which handles different data types beyond pandas DataFrames. The original implementation used self._get_accessor().on_delete(getattr(state, var_name), payload) which properly delegates to type-specific handlers. This change breaks support for non-DataFrame data types that were previously supported through the accessor pattern.

Suggested change

var = self._get_var_from_name(var_name)

df = self._get_var_from_name(var_name)

if isinstance(df, pd.DataFrame):

idx = payload.get("index")

if idx is None:

_warn("No index provided for deletion.")

return

try:

df = df.drop(df.index[int(idx)])

df.reset_index(drop=True, inplace=True)

self._set_var_in_name(var_name, df)

except (ValueError, IndexError, KeyError):

if idx in df.index:

df = df.drop(idx)

df.reset_index(drop=True, inplace=True)

self._set_var_in_name(var_name, df)

else:

_warn(f"Row '{idx}' not found in DataFrame.")

else:

_warn(f"Variable '{var_name}' is not a DataFrame.")

self._get_accessor().on_delete(getattr(state, var_name), payload)

Copilot · 2025-11-24T15:49:44Z

taipy/gui/gui.py

+            df = self._get_var_from_name(var_name)
+            if isinstance(df, pd.DataFrame):
+                new_row = payload.get("row", {})
+                df = pd.concat([df, pd.DataFrame([new_row], columns=df.columns)], ignore_index=True)
+                self._set_var_in_name(var_name, df)
+            else:
+                _warn(f"Variable '{var_name}' is not a DataFrame.")


The new implementation bypasses the existing data accessor architecture (self._get_accessor()) which handles different data types beyond pandas DataFrames. The original implementation used self._get_accessor().on_add(getattr(state, var_name), payload, new_row) which properly delegates to type-specific handlers. This change breaks support for non-DataFrame data types and ignores the new_row parameter that was part of the function signature.

Suggested change

df = self._get_var_from_name(var_name)

if isinstance(df, pd.DataFrame):

new_row = payload.get("row", {})

df = pd.concat([df, pd.DataFrame([new_row], columns=df.columns)], ignore_index=True)

self._set_var_in_name(var_name, df)

else:

_warn(f"Variable '{var_name}' is not a DataFrame.")

setattr(state, var_name, self._get_accessor().on_add(getattr(state, var_name), payload, new_row))

Copilot · 2025-11-24T15:49:44Z

taipy/gui/gui.py

+                    df = df.drop(df.index[int(idx)])
+                    df.reset_index(drop=True, inplace=True)
+                    self._set_var_in_name(var_name, df)
+                except (ValueError, IndexError, KeyError):
+                    if idx in df.index:
+                        df = df.drop(idx)
+                        df.reset_index(drop=True, inplace=True)
+                        self._set_var_in_name(var_name, df)
+                    else:
+                        _warn(f"Row '{idx}' not found in DataFrame.")


The delete logic tries two different approaches with exception handling, but the proper implementation (as seen in pandas_data_accessor.py line 562) uses df.drop(self._get_index_value(payload.get("index", 0))) directly. The new implementation's fallback logic (lines 1980-1986) is overly complex and doesn't handle the case where the index is positional vs. label-based correctly. The original accessor implementation handles this properly through _get_index_value().

Suggested change

df = df.drop(df.index[int(idx)])

df.reset_index(drop=True, inplace=True)

self._set_var_in_name(var_name, df)

except (ValueError, IndexError, KeyError):

if idx in df.index:

df = df.drop(idx)

df.reset_index(drop=True, inplace=True)

self._set_var_in_name(var_name, df)

else:

_warn(f"Row '{idx}' not found in DataFrame.")

index_value = self._get_index_value(idx)

df = df.drop(index_value)

df.reset_index(drop=True, inplace=True)

self._set_var_in_name(var_name, df)

except (ValueError, IndexError, KeyError):

_warn(f"Row '{idx}' not found in DataFrame.")

Copilot · 2025-11-24T15:49:44Z

taipy/gui/test_table.py

+    # Mock a variable in case Taipy uses an internal dict
+    if not hasattr(gui, "_variables"):
+        gui._variables = {}
+    gui._variables["dummy"] = "mocked_value"


Missing import for pandas in the test file. The test mocks a value as "mocked_value" (a string) but table_on_delete expects a pandas DataFrame (line 1971 checks isinstance(df, pd.DataFrame)). This test will always take the warning path and never actually test the delete functionality.

FredLL-Avaiga · 2025-11-28T09:53:42Z

Hi @ibrahim307le
I'm sorry but I won't review this PR if you don't take my earlier comment into consideration.
I'm considering closing this PR as it is very far from the standards we aim for.

…_delete now use state

FredLL-Avaiga

We cannot accept this PR as it is.
You need to take the comments into consideration.

Python test should be in the tests directory.
pandas should not be used in gui.py
the changes to setup.py or pyproject.toml are not relevant
the branch is not up-to-date

Without real changes, this PR will be closed

Fix: table delete callback error in Taipy GUI

fc0df32

ibrahim307le mentioned this pull request Nov 13, 2025

[🐛 BUG] Default table "editable" behaviour broken when df has index #2747

Open

6 tasks

arcanaxion linked an issue Nov 14, 2025 that may be closed by this pull request

[🐛 BUG] Default table "editable" behaviour broken when df has index #2747

Open

6 tasks

arcanaxion assigned FabienLelaquais Nov 14, 2025

FredLL-Avaiga requested changes Nov 24, 2025

View reviewed changes

FredLL-Avaiga requested review from FabienLelaquais and Copilot November 24, 2025 15:43

FredLL-Avaiga assigned ibrahim307le and unassigned FabienLelaquais Nov 24, 2025

Copilot started reviewing on behalf of FredLL-Avaiga November 24, 2025 15:44 View session

FredLL-Avaiga added 🖰 GUI Related to GUI 💥Malfunction Addresses an identified problem. 🟩 Priority: Low Low priority and doesn't need to be rushed 📝Release Notes Impacts the Release Notes or the Documentation in general Gui: Back-End labels Nov 24, 2025

Copilot finished reviewing on behalf of FredLL-Avaiga November 24, 2025 15:46

Copilot AI reviewed Nov 24, 2025

View reviewed changes

ibrahim307le requested a review from FredLL-Avaiga November 28, 2025 09:20

ibrahim307le added 2 commits November 28, 2025 15:40

Fix: updated _get_var_from_name and _set_var_in_name, table_on_add/on…

99e7684

…_delete now use state

Temporary commit: update pyproject.toml and remove setup.py

35f851e

FredLL-Avaiga requested changes Dec 1, 2025

View reviewed changes

		#This function sets the value of a specific cell in the tabular dataset stored in
		var_name, typically bound to the data property of a table control.

Fix: table delete callback error in Taipy GUI #2768

Are you sure you want to change the base?

Fix: table delete callback error in Taipy GUI #2768

Conversation

ibrahim307le commented Nov 13, 2025

Uh oh!

FabienLelaquais commented Nov 14, 2025

Uh oh!

ibrahim307le commented Nov 16, 2025

Uh oh!

FredLL-Avaiga left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

FredLL-Avaiga commented Nov 28, 2025

Uh oh!

FredLL-Avaiga left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FredLL-Avaiga left a comment •

edited

Loading

FredLL-Avaiga left a comment •

edited

Loading