refactor: remove deprecated interfaces [ready for review] by flo117 · Pull Request #312 · eclipse-kiso-testing/kiso-testing

flo117 · 2023-06-19T09:07:57Z

AuxiliaryCommon, MpAuxiliaryInterface, SimpleAuxiliaryInterface and AuxiliaryInterface have been removed. DTAuxiliaryInterface has been renamed to AuxiliaryInterface

flo117 · 2023-06-19T09:08:53Z

Removal of MpAuxiliaryInterface shall be discussed in next sync meeting.

sebastianpfischer · 2023-06-21T08:32:12Z

The review may take ages.... 45 files to review ^^

BKaDamien · 2023-06-21T08:56:34Z

AuxiliaryCommon, MpAuxiliaryInterface, SimpleAuxiliaryInterface and AuxiliaryInterface have been removed. DTAuxiliaryInterface has been renamed to AuxiliaryInterface

You maybe add some additionnal breaking changes by changing DTAuxiliaryInterface to AuxiliaryInterface. For any reason if a user want to use the interface for typehint, the merge of this branch will break the user's code.
I don't understand the renaming

sebclrsn · 2023-07-03T14:51:04Z

I would have expected the AuxiliaryInterface to be implemented here now, just like the Connector and CChannel interfaces

The dt_auxiliary module would then only contain the DTAuxiliaryInterface which will raise a warning

sebclrsn · 2023-07-17T13:47:16Z

            active polling.
            Note1: the data will automatically be saved
-            Note2: if proxy usage, all connectors should be 'CCMpProxy'
+            Note2: if proxy usage, all connectors should be 'CCProxy'


Didn't see that one... We'll still need to enable multiprocessing for this auxiliary, maybe by giving the possibility to see the Queue class of the CCProxy (queue.Queue or multiprocessing.Queue)

sebclrsn · 2023-07-17T13:49:17Z

-            log.internal_debug(f"called create_instance on {name}")
-        #################################################################################
-        elif not inst.is_instance and auto_start:
+        if not inst.is_instance and auto_start:


As all auxiliaries now inherit from the same interface, we could remove the method existence check above and below to just do:

if not inst.is_instance and auto_start: inst.start()

sebclrsn · 2023-07-17T13:51:51Z

        config["connectors"]["channel1"]["config"]["processing"] = True
-        cc_class = CCMpProxy
-        aux_class = MpProxyAuxiliary
+        cc_class = CCProxy


This should also be updated in config_registry.py

sebclrsn

Still some open points to clarify, either we merge this and tackle the leftover issues afterwards or we include them in this PR

sebclrsn · 2023-08-14T12:58:36Z

+Double Threaded based Auxiliary Interface
+*****************************************

 :module: auxiliary

-:synopsis: base auxiliary interface
+:synopsis: common double threaded based auxiliary interface

 .. currentmodule:: pykiso

 """


Auxiliary common Interface Definition ***************************************** :module: auxiliary :synopsis: base auxiliary interface .. currentmodule:: pykiso """

I think we can keep the original docstring

sebclrsn · 2023-08-14T13:01:56Z

Just be careful when dealing with the merge conflicts to include the latest changes in the DTAuxiliaryInterface :)

sebastianpfischer · 2023-08-16T12:57:13Z

-        self.multiprocess = multiprocess
        self.cursor = 0
        self.log_folder_path = log_folder_path
+        self.multiprocess = multiprocess


Why do we keep it here?

in line 143 it is checked if the channel is a CCProxy and if multiprocess was set to true.

I still do not understand, we removed the multiprocessing, why not removing it from here too? What is the UC?

@flo117 , do you have an update for here?

yes, sorry for late reply. It would lead to a breaking change in the cchannel, in which the user can select multiprocessing

sebastianpfischer · 2023-09-05T09:57:12Z

-        self.multiprocess = multiprocess
        self.cursor = 0
        self.log_folder_path = log_folder_path
+        self.multiprocess = multiprocess


@flo117 , do you have an update for here?

sebastianpfischer · 2023-10-06T08:58:16Z

@flo117 , how are we doing here :)

codecov · 2023-10-11T12:11:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0a2685e) 97.11% compared to head (00b1f48) 96.98%.
Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
- Coverage   97.11%   96.98%   -0.14%     
==========================================
  Files          85       81       -4     
  Lines        6866     6426     -440     
==========================================
- Hits         6668     6232     -436     
+ Misses        198      194       -4

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

flo117 · 2023-10-11T12:20:58Z

@flo117 , how are we doing here :)

i removed all multiprocessing and done a rebase

sebastianpfischer requested review from BKaDamien, sebastianpfischer and sebclrsn June 21, 2023 08:30

BKaDamien suggested changes Jun 21, 2023

View reviewed changes

Comment thread src/pykiso/interfaces/dt_auxiliary.py Outdated

Comment thread src/pykiso/test_setup/dynamic_loader.py Outdated

sebclrsn reviewed Jun 23, 2023

View reviewed changes

Comment thread docs/advanced_usage/how_to_auxiliary.rst Outdated

sebclrsn requested changes Jun 23, 2023

View reviewed changes

Comment thread src/pykiso/test_setup/dynamic_loader.py Outdated

flo117 force-pushed the remove_deprecated_interface branch from 785f542 to 00323c1 Compare July 3, 2023 09:35

flo117 requested review from BKaDamien and sebclrsn July 3, 2023 09:49

BKaDamien suggested changes Jul 3, 2023

View reviewed changes

Comment thread docs/advanced_usage/how_to_auxiliary.rst

flo117 requested a review from BKaDamien July 3, 2023 12:29

BKaDamien approved these changes Jul 3, 2023

View reviewed changes

sebclrsn reviewed Jul 3, 2023

View reviewed changes

Comment thread src/pykiso/lib/auxiliaries/proxy_auxiliary.py Outdated

flo117 requested a review from sebclrsn July 14, 2023 07:56

sebclrsn reviewed Jul 17, 2023

View reviewed changes

flo117 requested review from BKaDamien and sebclrsn August 14, 2023 09:04

sebclrsn requested review from Pog3k and sebclrsn and removed request for sebclrsn August 14, 2023 09:05

sebclrsn reviewed Aug 14, 2023

View reviewed changes

sebclrsn approved these changes Aug 14, 2023

View reviewed changes

sebastianpfischer reviewed Aug 16, 2023

View reviewed changes

sebastianpfischer requested changes Sep 5, 2023

View reviewed changes

flo117 force-pushed the remove_deprecated_interface branch 2 times, most recently from e39f7e7 to c83f1bd Compare October 11, 2023 12:09

flo117 requested review from sebastianpfischer and sebclrsn October 11, 2023 12:19

sebastianpfischer approved these changes Oct 25, 2023

View reviewed changes

Pog3k approved these changes Oct 25, 2023

View reviewed changes

flo117 and others added 3 commits November 8, 2023 14:59

refactor: remove deprecated interfaces

d6226f0

refactor: remove remaining multiprocessing

512a31b

fix: reformatted

00b1f48

flo117 force-pushed the remove_deprecated_interface branch from 542e2f0 to 00b1f48 Compare November 8, 2023 14:01

sebastianpfischer merged commit 61e219e into eclipse-kiso-testing:master Nov 22, 2023

Conversation

flo117 commented Jun 19, 2023

Uh oh!

flo117 commented Jun 19, 2023

Uh oh!

sebastianpfischer commented Jun 21, 2023

Uh oh!

BKaDamien commented Jun 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebclrsn left a comment

Choose a reason for hiding this comment

Uh oh!

sebclrsn Aug 14, 2023 • 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!

sebclrsn commented Aug 14, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianpfischer commented Oct 6, 2023

Uh oh!

codecov Bot commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

flo117 commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sebclrsn Aug 14, 2023 •

edited

Loading

codecov Bot commented Oct 11, 2023 •

edited

Loading