Skip to content

Remove redundant properties#878

Open
aversey wants to merge 9 commits into
logicalclocks:mainfrom
aversey:properties
Open

Remove redundant properties#878
aversey wants to merge 9 commits into
logicalclocks:mainfrom
aversey:properties

Conversation

@aversey
Copy link
Copy Markdown
Contributor

@aversey aversey commented Mar 20, 2026

This PR adds/fixes/changes...

  • please summarize your changes to the code
  • and make sure to include all changes to user-facing APIs

JIRA Issue: -

Priority for Review: -

Related PRs: -

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Tests on VM

Checklist For The Assigned Reviewer:

- [ ] Checked if merge conflicts with master exist
- [ ] Checked if stylechecks for Java and Python pass
- [ ] Checked if all docstrings were added and/or updated appropriately
- [ ] Ran spellcheck on docstring
- [ ] Checked if guides & concepts need to be updated
- [ ] Checked if naming conventions for parameters and variables were followed
- [ ] Checked if private methods are properly declared and used
- [ ] Checked if hard-to-understand areas of code are commented
- [ ] Checked if tests are effective
- [ ] Built and deployed changes on dev VM and tested manually
- [x] (Checked if all type annotations were added and/or updated appropriately)

@aversey aversey requested review from Copilot and manu-sj March 20, 2026 14:13
@aversey aversey self-assigned this Mar 20, 2026
Copy link
Copy Markdown
Contributor

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 modernizes several Python API/data-model classes by removing redundant private attributes plus trivial property getters/setters, replacing them with direct (typed) attributes and attribute docstrings, and bumps hopsworks-apigen accordingly.

Changes:

  • Bump hopsworks-apigen dependency (and build extra) from 1.0.4 to 1.0.5.
  • Replace many _x + @property patterns with direct attributes (plus type annotations) across hsfs/hopsworks_common classes.
  • Update serialization (to_dict/repr) call sites to use the new attribute names.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
python/pyproject.toml Bumps hopsworks-apigen runtime + build dependency to >=1.0.5,<2.0.0.
python/hsfs/training_dataset_split.py Switches split fields from private vars/properties to direct attributes and updates to_dict.
python/hsfs/statistics_config.py Adds typed public attributes with attribute docstrings; removes property wrappers; updates to_dict/__repr__.
python/hsfs/feature_view.py Introduces typed schema attribute and replaces _features; refactors featurestore id handling (currently with an inconsistency/typo).
python/hsfs/feature_group_commit.py Switches several commit metadata fields to direct attributes and updates to_dict.
python/hsfs/feature_group.py Moves feature group metadata/schema to typed public attributes; updates references in query construction and serialization.
python/hsfs/core/training_dataset_job_conf.py Converts job conf fields to direct attributes and updates to_dict.
python/hsfs/core/online_store_sql_engine.py Converts prepared statements storage to a typed public attribute; removes property wrappers.
python/hsfs/constructor/serving_prepared_statement.py Converts core fields to direct attributes and updates to_dict/__repr__.
python/hsfs/constructor/query.py Converts left FG time-travel window fields to direct attributes and updates to_dict.
python/hopsworks_common/util.py Converts AsyncTask’s event/result to direct attributes; removes property wrappers.
python/hopsworks_common/core/sink_job_configuration.py Converts multiple config fields to direct typed attributes; updates @public(...) export paths and to_dict.
python/hopsworks_common/core/ingestion_job_conf.py Converts ingestion job conf fields to direct attributes and updates to_dict.

# Check if query has any ambiguous columns and print warning in these cases:
query.check_and_warn_ambiguous_features()
self._featurestore_id = featurestore_id
self.schematore_id = featurestore_id
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

schematore_id looks like a typo and introduces a second source of truth for the feature store id alongside _feature_store_id. Because featurestore_id setters/update_from_response_json update schematore_id only, methods that still use _feature_store_id (e.g., alert APIs) can become inconsistent. Use a single attribute for this (prefer _feature_store_id) and have the featurestore_id property read/write that consistently, updating all call sites accordingly.

Suggested change
self.schematore_id = featurestore_id

Copilot uses AI. Check for mistakes.
Comment on lines 4771 to +4777
def featurestore_id(self) -> int:
"""Feature store id."""
return self._featurestore_id
return self.schematore_id

@featurestore_id.setter
def featurestore_id(self, id: int | None) -> None:
self._featurestore_id = id
self.schematore_id = id
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The featurestore_id property currently reads/writes schematore_id only, which can diverge from _feature_store_id used elsewhere in the class. After consolidating the backing field, ensure this property remains the single consistent way to get/set the feature store id (including during update_from_response_json).

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +36
columns: list
"""Specify a subset of columns to compute statistics for."""
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

columns is annotated as plain list, which loses the element type information and undermines the benefit of the newly added attribute type hints. Consider using a more specific type (likely list[str]) and aligning __init__ parameter annotations accordingly so the public API is fully typed.

Copilot uses AI. Check for mistakes.
Comment thread python/pyproject.toml
"protobuf>=4.25.4,<5.0.0", # ^4.25.4
"packaging", # ^21.0
"hopsworks-apigen>=1.0.4,<2.0.0",
"hopsworks-apigen>=1.0.5,<2.0.0",
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

This change bumps hopsworks-apigen and (based on the diff) alters how user-facing attributes/properties are exposed. The PR description currently doesn’t summarize these user-facing API changes; please update it to explicitly call out the public API impact and the dependency bump rationale.

Copilot uses AI. Check for mistakes.
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