refactor: refactor try_network_action() (DEV-1844) #325

jnussbaum · 2023-03-13T08:09:35Z

linear · 2023-03-13T08:09:37Z

DEV-1844 DSP-TOOLS: refactor try_network_action()

… exception

jnussbaum · 2023-03-22T12:17:05Z

src/dsp_tools/utils/xml_upload.py

@@ -251,7 +251,7 @@ def _stash_circular_references(
                                stashed_resptr_props[res][link_prop].append(str(value.value))
                        link_prop.values.remove(value)
            else:
-                logger.exception("ERROR in remove_circular_references(): link_prop.valtype is neither text nor resptr.")
+                logger.error("ERROR in remove_circular_references(): link_prop.valtype is neither text nor resptr.")


logging.exception() is a shorthand for logging.error(exc_info=True), which should only be called inside an except block.

jnussbaum · 2023-03-22T12:18:44Z

src/dsp_tools/utils/xml_upload.py

+        proj_context = try_network_action(lambda: ProjectContext(con=con))
+    except BaseError:
+        logger.error("Unable to retrieve project context from DSP server", exc_info=True)
+        raise UserError("Unable to retrieve project context from DSP server") from None


Until now, the error "Unable to retrieve project context from DSP server" escalated uncontrolled. Now, it is logged incl. stacktrace, and a user-friendly UserError is raised instead

jnussbaum · 2023-03-22T12:20:00Z

src/dsp_tools/utils/xml_upload.py

-    try_network_action(failure_msg="Unable to login to DSP server", action=lambda: con.login(user, password))
-    proj_context = try_network_action(failure_msg="Unable to retrieve project context from DSP server",
-                                      action=lambda: ProjectContext(con=con))
+    con = login(server=server, user=user, password=password)


shared.login() is a convenience method that creates a Connection object and makes the login

jnussbaum · 2023-03-22T17:48:53Z

@BalduinLandolt One thing that could be problematic is the manner how multiple-line log messages are written. It looks like this:

2023-03-21 13:47:12,248   shared.py            ERROR    The XML file contains the following syntax error: Opening and ending tag mismatch: xml line 69 and text, line 69, column 109
Traceback (most recent call last):
  File "/Users/nussbaum/Desktop/dsp-tools/src/dsp_tools/utils/shared.py", line 115, in validate_xml_against_schema
    doc = etree.parse(source=input_file)
  File "src/lxml/etree.pyx", line 3541, in lxml.etree.parse
  File "src/lxml/parser.pxi", line 1879, in lxml.etree._parseDocument
  File "src/lxml/parser.pxi", line 1905, in lxml.etree._parseDocumentFromURL
  File "src/lxml/parser.pxi", line 1808, in lxml.etree._parseDocFromFile
  File "src/lxml/parser.pxi", line 1180, in lxml.etree._BaseParser._parseDocFromFile
  File "src/lxml/parser.pxi", line 618, in lxml.etree._ParserContext._handleParseResultDoc
  File "src/lxml/parser.pxi", line 728, in lxml.etree._handleParseResult
  File "src/lxml/parser.pxi", line 657, in lxml.etree._raiseParseError
  File "testdata/xml-data/test-data-systematic.xml", line 69
lxml.etree.XMLSyntaxError: Opening and ending tag mismatch: xml line 69 and text, line 69, column 109
2023-03-21 13:47:41,226   xml_upload.py        INFO     Method call xml_upload(input_file='testdata/xml-data/test-data-systematic.xml', server='http://0.0.0.0:3333', user='root@example.com', imgdir='.', sipi='http://0.0.0.0:1024', verbose=False, incremental=False, save_metrics=False)
2023-03-21 13:47:46,894   shared.py            ERROR    XML-tags are not allowed in text properties with encoding=utf8. The following resources of your XML file violate this rule:
 - line 69: resource 'INV39A4413ADDA3452AAA8A1AC58B88D3D4', property ':hasId'
NoneType: None

By accident, I saw this today, and liked it much more than my solution:

2023-03-22 17:36:42 [INFO]   ---------------------------------------------
2023-03-22 17:36:42 [INFO]   --- GitHub Actions Multi Language Linter ----
2023-03-22 17:36:42 [INFO]    - Image Creation Date:[2023-01-04T06:59:26Z]
2023-03-22 17:36:42 [INFO]    - Image Revision:[985ef206aaca4d560cb9ee2af2b42ba44adc1d55]
2023-03-22 17:36:42 [INFO]    - Image Version:[985ef206aaca4d560cb9ee2af2b42ba44adc1d55]
2023-03-22 17:36:42 [INFO]   ---------------------------------------------
2023-03-22 17:36:42 [INFO]   ---------------------------------------------
2023-03-22 17:36:42 [INFO]   The Super-Linter source code can be found at:
2023-03-22 17:36:42 [INFO]    - https://github.com/github/super-linter
2023-03-22 17:36:42 [INFO]   ---------------------------------------------

I'm motivated to look for a solution how multi-line messages can be split up, and every line logged separately, so that the layout is more beautiful. What do you think about that?

BalduinLandolt

Overall looking good. I remarked some minor things.
I personally don't like the duplication of the error messages all over the place. I think I would prefer the pattern

msg = "..."
print(msg)
log.error(msg)

Regarding how your logs look. I would definitely leave it as is! This is exactly how multi-line logs should be looking (even though I understand your dislike for the aesthetics of it).
But when you have something like the counter example you showed, that's IMO actually bad practice, because there you have multiple log messages for one bit of information, some of them even containing only --------------. Imagine a service like DataDog that parses your logs for you - there you would then have to look at multiple log messages, to see what's going on.

BalduinLandolt · 2023-03-23T07:17:34Z

src/dsp_tools/utils/project_create.py

-        print(f"\tWARNING: Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP "
-              f"server. Updating it...")
-        success = False
+        project_remote: Project = try_network_action(lambda: project_local.read())


Suggested change

project_remote: Project = try_network_action(lambda: project_local.read())

project_remote: Project = try_network_action(project_local.read)

This is beside the topic, but just because it caught my eye: I'm not 100% confident, but I think you don't need the lambda here, if you just pass the method as a function

When I introduced this, I made experiments and I found out that the lambda is necessary. It also makes sense from a theoretical point of view:
try_network_action(project_local.read) passes the result of read() to the method, which is exactly what we don't want.
On the other hand, try_network_action(lambda: project_local.read()) passes the read() method itself to the method, and that's what we want

no, as far as I know, the difference between
try_network_action(project_local.read) and
try_network_action(project_local.read())
should be that one passes the function and the other the evaluation of the function - and the lambda is also just a function pointing to project_local.read.

Ah, now I see, you have omitted the () in your first comment. Sorry, I wasn't attentive enough.

That's actually a good point. You're probably right and I could try that out. Now, I already merged, but I will keep it in mind.

no problem. Sure, give it a try at some point, if you like... but this wasn't scope of the present PR anyways, and it works the way it is, so really no rush to change

BalduinLandolt · 2023-03-23T07:19:11Z

src/dsp_tools/utils/project_create.py

+        print(f"\tWARNING: Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")
+        logger.warning(f"Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")


Suggested change

print(f"\tWARNING: Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")

logger.warning(f"Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")

msg = f"Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it..."

print(f"\tWARNING: {msg}")

logger.warning(msg)

just a thought, how to make this duplication a bit nicer

BalduinLandolt · 2023-03-23T07:22:48Z

src/dsp_tools/utils/project_create.py

+        err_msg = f"Cannot create project '{project_definition['project']['shortname']}' " \
+                  f"({project_definition['project']['shortcode']}) on DSP server."


I would personally put things like {project_definition['project']['shortname']} in a separate variable. It adds some extra lines but I think the code would still be more readable.

BalduinLandolt · 2023-03-23T07:26:28Z

src/dsp_tools/utils/shared.py


    Args:
        server: URL of the DSP server to connect to
        user: Username (e-mail)
        password: Password of the user

    Raises:
-        BaseError if the login fails
+        UserError if the login fails permanently


what do you mean by "fails permanently"? I guess you want to stress that it failed despite multiple retries. But I feel just "fails" is clear enough

jnussbaum added 2 commits March 10, 2023 19:02

refactor everywhere except xml_upload

d73a6b6

finish xmlupload

c861b5c

jnussbaum self-assigned this Mar 13, 2023

jnussbaum and others added 9 commits March 14, 2023 13:19

Merge branch 'main' into wip/dev-1844-refactor-try-network-action

6aa703c

Merge branch 'main' into wip/dev-1844-refactor-try-network-action

fe4bf38

Merge branch 'main' into wip/dev-1844-refactor-try-network-action

2b96bd3

Merge branch 'main' into wip/dev-1844-refactor-try-network-action

05f0e0a

Merge branch 'main' into wip/dev-1844-refactor-try-network-action

cde0fa6

fix

265739a

fix test

e13cefe

Merge branch 'main' into wip/dev-1844-refactor-try-network-action

e8d28c3

don't make logging inside try_network_action(), but when catching the…

90770d2

… exception

jnussbaum changed the title ~~chore: refactor try network action (DEV-1844)~~ refactor: refactor try_network_action() (DEV-1844) Mar 22, 2023

jnussbaum commented Mar 22, 2023

View reviewed changes

jnussbaum requested a review from BalduinLandolt March 22, 2023 13:13

edit

c4c6f95

BalduinLandolt approved these changes Mar 23, 2023

View reviewed changes

jnussbaum merged commit 896586f into main Mar 23, 2023
2 checks passed

jnussbaum deleted the wip/dev-1844-refactor-try-network-action branch March 23, 2023 10:09

daschbot mentioned this pull request Mar 23, 2023

chore: release 2.2.1 #342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: refactor try_network_action() (DEV-1844) #325

refactor: refactor try_network_action() (DEV-1844) #325

jnussbaum commented Mar 13, 2023

linear bot commented Mar 13, 2023

jnussbaum Mar 22, 2023

jnussbaum Mar 22, 2023

jnussbaum Mar 22, 2023

jnussbaum commented Mar 22, 2023

BalduinLandolt left a comment

BalduinLandolt Mar 23, 2023

jnussbaum Mar 23, 2023

BalduinLandolt Mar 23, 2023

jnussbaum Mar 23, 2023

BalduinLandolt Mar 23, 2023

BalduinLandolt Mar 23, 2023

BalduinLandolt Mar 23, 2023

BalduinLandolt Mar 23, 2023

	project_remote: Project = try_network_action(lambda: project_local.read())
	project_remote: Project = try_network_action(project_local.read)

		print(f"\tWARNING: Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")
		logger.warning(f"Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")

-        print(f"\tWARNING: Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")
-        logger.warning(f"Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it...")
+        msg = f"Project '{project_remote.shortname}' ({project_remote.shortcode}) already exists on the DSP server. Updating it..."
+        print(f"\tWARNING: {msg}")
+        logger.warning(msg)

		err_msg = f"Cannot create project '{project_definition['project']['shortname']}' " \
		f"({project_definition['project']['shortcode']}) on DSP server."

refactor: refactor try_network_action() (DEV-1844) #325

refactor: refactor try_network_action() (DEV-1844) #325

Conversation

jnussbaum commented Mar 13, 2023

linear bot commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnussbaum commented Mar 22, 2023

BalduinLandolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment