Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pylint Errors Due to Compatibility Issues with Updated Sedna in CI Environment #152

Open
FuryMartin opened this issue Sep 27, 2024 · 9 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@FuryMartin
Copy link
Contributor

FuryMartin commented Sep 27, 2024

What happened:
CI will attempt to install Sedna from examples/resources/third_party/ when setting up the build environment. But If a new PR uses an updated version of Sedna and does not include the newer wheel in this directory, it will result in interface inconsistencies, causing Pylint to detect module errors (unexpected-keyword-arg, no-name-in-module, etc).

Possible errors may be reported:

# Note: Encounter when original interface is changed
************* Module core.testcasecontroller.algorithm.paradigm.base
core/testcasecontroller/algorithm/paradigm/base.py:130:19: E1123: Unexpected keyword argument 'cloud' in constructor call (unexpected-keyword-arg)
core/testcasecontroller/algorithm/paradigm/base.py:130:19: E1123: Unexpected keyword argument 'LCReporter_enable' in constructor call (unexpected-keyword-arg)

# Note: Encounter when new interface is added
************* Module core.testenvmanager.dataset.dataset
core/testenvmanager/dataset/dataset.py:21:0: E0611: No name 'JsonlDataParse' in module 'sedna.datasources' (no-name-in-module)
core/testenvmanager/dataset/dataset.py:21:0: E0611: No name 'JSONDataInfoParse' in module 'sedna.datasources' (no-name-in-module)

What you expected to happen:
The code should pass Pylint checks without any errors, and the new interfaces from Sedna should be properly recognized.

How to reproduce it (as minimally and precisely as possible):

  1. Develop ianvs core/ with a newer version of Sedna that includes new features or bug fixes.
  2. Avoid adding the newer wheel of Sedna into examples/resources/third_party/.
  3. Submit a pull request and run continuous integration (CI).

Anything else we need to know?:
Related PRs: #144, #149 and other PRs associated with Sedna update

@FuryMartin FuryMartin added the kind/bug Categorizes issue or PR as related to a bug. label Sep 27, 2024
@FuryMartin
Copy link
Contributor Author

FuryMartin commented Sep 27, 2024

Here is my analysis of the issue.

There are two direct reasons for this issue:

  1. Ianvs needs to modify Sedna when introducing some new features;
  2. When submitting a PR, Ianvs must use the updated Wheel to pass CI's Pylint checks.

To pass the Pylint check, contributors can upload their modified Sedna Wheel to examples/resources/third_party/.

However, this approach has significant risks:

  1. The Wheel uploaded to Ianvs is a binary file, and Git cannot track internal changes in detail, making it impossible to review modifications made to Sedna.
  2. If multiple contributors have modified Sedna, their changes cannot be merged into a single unified Wheel, leading to conflicts.
  3. Even if they can be merged into one unified Wheel, it is likely to diverge from the main branch of Sedna. Actually, the diverge has already occured, this issue discusses the differences in the LifelongLearning interface between the official Sedna and Ianvs' version.

One possible solution is for contributors to submit their modifications as PRs to the official Sedna repository; once officially merged, they can then introduce the new Wheel into Ianvs.

The challenge lies in: since it's difficult to verify whether these modifications will affect other features of the official Sedna repository, contributors' PR may be difficult to be merged.

This actually reflects the flaws in current dependency management within Ianvs.

See #132 for more details about these flaws (especially this part)

@FuryMartin
Copy link
Contributor Author

This issue is a little complex and may require collaboration between the Sedna and Ianvs projects.

It exceeds my personal capabilities to resolve. I need support from the community maintainers.

@MooreZheng @hsj576

@MooreZheng
Copy link
Collaborator

@tangming1996 @hsj576 @jaypume any advice?

@FuryMartin
Copy link
Contributor Author

FuryMartin commented Sep 27, 2024

Here is a possible solution for addressing #152 and #132.

Fix PR Problem related to changes of Sedna #152

We assume that for new features, developers have thoroughly validated their changes on their modified Sedna when submitting PRs and have ensured interface compatibility as much as possible.

For Sedna

  1. Maintainers can create an ianvs branch in the official Sedna repository, allowing developers to modify lib/ via PR submissions to meet Ianvs's change requirements when introducing new features.
  2. The PRs submitted by developers will first undergo code review by Sedna's maintainers; those that meet merge criteria will be merged into the ianvs branch.
  3. If certain commits from the ianvs branch are also beneficial to the main branch, they can be cherry-picked into it.
  4. When main branch changes, the ianvs branch needs to merge it promptly to avoid prolonged divergence, which can be automated via Bot or Github Actions.

For Ianvs

  1. Ianvs adds sedna to requirements.txt using a method that installs from Sedna's repository:
git+https://github.com/KubeEdge/sedna@ianvs#subdirectory=lib/
  1. When developers submit PRs to Ianvs, they must first ensure their changes to Sedna have been merged into its ianvs branch.
  2. Ianvs' CI will install the latest version of Sedna on ianvs branch via pip install -r requirements.txt. If everything goes fine, CI will pass all tests successfully.

Futher improvements of ianvs' dependency management #132

Dependencies of Core

Considering that Sedna itself is a third-party dependency and is no different from other third-party dependencies like pretty_table; therefore, it is most appropriate for all packages needed by core/ to be listed in requirements.txt, which means examples/resources can be removed.

Dependencies of Examples

Different examples have different dependencies. Each example should add a requirements.txt to their example and include installation instructions in its README.md.

Benefits

With such adjustments, the overall dependency management of Ianvs will be simpler and clearer.

Specifically, the benefits are:

  • Resolve Sedna's change and synchronization issues.
  • The dependencies of Core and Examples will become independent.
  • All dependencies will be installed using pip to achieve synchronization with upstream versions.

Please feel free to share your comments.

@MooreZheng
Copy link
Collaborator

MooreZheng commented Oct 15, 2024

@FuryMartin @IcyFeather233 @hsj576 Given the limited time of the OSPP project, my suggestion is to revise related codes in a copy of Sedna and build another specific wheel in examples/resources for current usage.

As for long-term improvement, creating a branch of Sedna for Ianvs users looks fine to me. For that issue, please get in touch with @tangming1996 for consideration. Once @tangming1996 can live with that, @FuryMartin can ask the current maintainer @jaypume for help. The solution of requirement.txt also looks good to me.

@FuryMartin
Copy link
Contributor Author

FuryMartin commented Nov 7, 2024

@FuryMartin @IcyFeather233 @hsj576 Given the limited time of the OSPP project, my suggestion is to revise related codes in a copy of Sedna and build another specific wheel in examples/resources for current usage.

In #149, I provided a modified Sedna package in third_party/ and move the old sedna-0.4.1-py3-none-any.whl to third_party_bk/. The source code can be found at https://github.com/FuryMartin/sedna/tree/jsonl.

@MooreZheng
Copy link
Collaborator

@FuryMartin @IcyFeather233 @hsj576 Given the limited time of the OSPP project, my suggestion is to revise related codes in a copy of Sedna and build another specific wheel in examples/resources for current usage.

In #149, I provided a modified Sedna package in third_party/ and move the old sedna-0.4.1-py3-none-any.whl to third_party_bk/. The source code can be found at https://github.com/FuryMartin/sedna/tree/jsonl.

Would the #149 PR be sufficient to close this issue?

@FuryMartin
Copy link
Contributor Author

Would the #149 PR be sufficient to close this issue?

Yes, #149 fixed the current CI errors.

However, we may encounter similar issue in the future. Shall I close this issue now?

@MooreZheng
Copy link
Collaborator

Would the #149 PR be sufficient to close this issue?

Yes, #149 fixed the current CI errors.

However, we may encounter similar issue in the future. Shall I close this issue now?

After several weeks, this issue can become vague to community members... In this case, @FuryMartin might want to conclude what could be done in the future. The previous comments might be helpful for summarization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

5 participants