akarneliuk / pygnmi

The pure Python implementation of the gNMI client.
https://training.karneliuk.com
BSD 3-Clause "New" or "Revised" License
127 stars 44 forks source link

Code assumes the use of 'with', should make this clear in the examples #18

Closed jbemmel closed 3 years ago

jbemmel commented 3 years ago

The 'with' construct in Python calls the enter and exit methods. The library assumes that, to a point that things only work using with: ...

akarneliuk commented 3 years ago

Hello @jbemmel ,

Thanks a lot for your comment. To be honest, I'm not sure I can follow up. Please, provide the details (e.g., code snippet and what exactly is not working or working not correctly).

Thanks, Anton

jbemmel commented 3 years ago

self.stub is assigned in enter__, see https://github.com/akarneliuk/pygnmi/blob/master/pygnmi/client.py#L102

Without 'with', this method is not called, and the code fails because '__stub' is not assigned.

It's the implicit calls to 'enter' and 'exit' that make or break the client code.

For the same reason, the library enforces a pattern of short-lived connections. One cannot connect() to the server at startup, and then keep a gNMI connection open for a long time; the exit call from 'with' closes the connection.

It can be a design choice (I don't particularly like it personally, but that's just me). However, it should be documented - it's not obvious, especially for people who are just getting started with Python

akarneliuk commented 3 years ago

Hello @jbemmel ,

Yeah, you are correct. It was an idea to create it using with ... as ... context manager, as I prefer such construction myself. However, there is nothing about about using connect method. I think, we can add that relatively quickly.

Best, Anton

slieberth commented 3 years ago

Hi Anton, hi jbemmel, basically jbemmel is right, that pygnmi uses 'with' construct, which has some limitations is you want to use long-lived connections ... however it is also possible to decompose the with method and use enter .

here the code sequence, I am using:

            try:
                host = (self.hostname, self.port)
                if self.path_cert:
                    self.gc = gNMIclient(
                        target=host,
                        debug=True,
                        username=self.username,
                        path_cert=self.path_cert,
                        gnmi_timeout=self.gnmi_timeout,
                        insecure=self.insecure)
                    self.gc.__enter__()
                else:
                    self.gc = gNMIclient(
                        target=host,
                        debug=True,
                        username=self.username,
                        password=self.password,
                        gnmi_timeout=self.gnmi_timeout,
                        insecure=self.insecure)
                    self.gc.__enter__()
            except Exception as exc:
                self.logger.error("error: %s\n %s",
                    exc, traceback.format_exc())
                self.logger.error(
                    'Error on line %s', sys.exc_info()[-1].tb_lineno)
                self.logger.warning("connect to %s port %s failed: %s",
                                    self.hostname, self.port, exc)

and for disconnect:

 def disconnect(self):
    try:
        self.gc.close()
        self.logger.info('del %s', self.gc)
        del self.gc
        self.logger.info('disconnect succeeded')
    except Exception as exc:
        self.logger.error('disconnect error: %s', exc)      

works well on my side for integrating pygnmi in an umbrella test tool ....

jbemmel commented 3 years ago

Hi Stefan,

Yes, you can call the implied 'with' methods of enter and exit manually, and implement long-lived connections that way. However, in my opinion that's still very much non-intuitive, and makes too many assumptions about the implementation (like calling enter without corresponding exit being ok) The "__" in the method names intends to mark them as private methods, not to be called directly (I believe)

I think a better option is to implement connect() and disconnect() - or close() - in pygnmi, and then have enter and exit call those methods. The current code simply replicates the code in enter and connect(), which is suboptimal - but it basically follows this idea

Regards, Jeroen

On Sat, Jul 10, 2021 at 5:39 AM Stefan Lieberth @.***> wrote:

Hi Anton, hi jbemmel, basically jbemmel is right, that pygnmi uses 'with' construct, which has some limitations is you want to use long-lived connections ... however it is also possible to decompose the with method and use enter .

here the code sequence, I am using:

        try:
            host = (self.hostname, self.port)
            if self.path_cert:
                self.gc = gNMIclient(
                    target=host,
                    debug=True,
                    username=self.username,
                    path_cert=self.path_cert,
                    gnmi_timeout=self.gnmi_timeout,
                    insecure=self.insecure)
                self.gc.__enter__()
            else:
                self.gc = gNMIclient(
                    target=host,
                    debug=True,
                    username=self.username,
                    password=self.password,
                    gnmi_timeout=self.gnmi_timeout,
                    insecure=self.insecure)
                self.gc.__enter__()

and for disconnect:

def disconnect(self): try: self.gc.close() self.logger.info('del %s', self.gc) del self.gc self.logger.info('disconnect succeeded') except Exception as exc: self.logger.error('disconnect error: %s', exc)

works well on my side for integrating pygnmi in an umbrella test tool ....

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/akarneliuk/pygnmi/issues/18#issuecomment-877615667, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPQAC4EN4OJQVC5ID7HFJLTXAPPBANCNFSM466HAWKQ .

slieberth commented 3 years ago

Hi Jereon,

Basically, I agree with you view that "a better option is to implement connect() and disconnect() - or close() - in pygnmi, and then have enter and exit call those methods"  ... however ... for my application, I found a working solution, using the existing code ...

I leave it to Anton to decide on a change ... from my side I could contribute to that change for code structuring and testing.

regards Stefan

Am 10.07.21 um 19:21 schrieb J vanBemmel:

Hi Stefan,

Yes, you can call the implied 'with' methods of enter and exit manually, and implement long-lived connections that way. However, in my opinion that's still very much non-intuitive, and makes too many assumptions about the implementation (like calling enter without corresponding exit being ok) The "__" in the method names intends to mark them as private methods, not to be called directly (I believe)

I think a better option is to implement connect() and disconnect() - or close() - in pygnmi, and then have enter and exit call those methods. The current code simply replicates the code in enter and connect(), which is suboptimal - but it basically follows this idea

Regards, Jeroen

On Sat, Jul 10, 2021 at 5:39 AM Stefan Lieberth @.***> wrote:

Hi Anton, hi jbemmel, basically jbemmel is right, that pygnmi uses 'with' construct, which has some limitations is you want to use long-lived connections ... however it is also possible to decompose the with method and use enter .

here the code sequence, I am using:

try: host = (self.hostname, self.port) if self.path_cert: self.gc = gNMIclient( target=host, debug=True, username=self.username, path_cert=self.path_cert, gnmi_timeout=self.gnmi_timeout, insecure=self.insecure) self.gc.enter() else: self.gc = gNMIclient( target=host, debug=True, username=self.username, password=self.password, gnmi_timeout=self.gnmi_timeout, insecure=self.insecure) self.gc.enter()

and for disconnect:

def disconnect(self): try: self.gc.close() self.logger.info('del %s', self.gc) del self.gc self.logger.info('disconnect succeeded') except Exception as exc: self.logger.error('disconnect error: %s', exc)

works well on my side for integrating pygnmi in an umbrella test tool ....

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/akarneliuk/pygnmi/issues/18#issuecomment-877615667, or unsubscribe

https://github.com/notifications/unsubscribe-auth/AAPQAC4EN4OJQVC5ID7HFJLTXAPPBANCNFSM466HAWKQ .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/akarneliuk/pygnmi/issues/18#issuecomment-877672060, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCRBDFPAMUPJSPKUF5GANLTXB6Q3ANCNFSM466HAWKQ.

akarneliuk commented 3 years ago

Hey @jbemmel , @slieberth ,

I've scrolled over some implementations of different libraries and figured out that return self.*** is quite an often implementation of the __enter__ method. So I think, it would make sense to simplify that we use return self.connect().

@slieberth , if you could do that, that would be just amazing from you.

Thanks, Anton

akarneliuk commented 3 years ago

Hello folks,

That is now implemented in 0.5.3, so I'm closing the issue.

Best, Anton