Improve error handling for some kinds of chip.get / chip.getkeys failures.

[WRansohoff]

I've run into a few situations where it has been difficult to debug the source of an error, because a generic exception is thrown at this line in a chip.get or chip.getkeys call:

keypathstr = ','.join(keypath)

The errors typically come from a None value in the keypath which gets passed into the method, along the lines of:

TypeError: sequence item 1: expected str instance, NoneType found

Usually, this happens because a previous chip.get call received a value of None when the build script was expecting a valid string. The easiest way to observe this kind of error is:

import siliconcompiler
siliconcompiler.Chip('my_design').run()

Tracking down the unexpectedly empty value from a generic Python error is difficult, so I think we should add some checks to make sure that the keypath is valid at the top of the chip.get and chip.getkeys functions.

With this change, the invalid key path will be printed out, which should help the user determine which value is missing. In the above example, the invalid path is ('flowgraph', None), which points towards the root cause that no EDA flow was selected.

[nmoroze]

This is a very helpful change, thanks! These errors have bugged me for a while, so it'll be nice to finally have this cleaned up.

I do think this PR could give us much broader benefit with just a bit more effort, and clean up some tech debt in the meantime.

First of all, I think we can get rid of all the keypathstr logic in get()/getkeys()/set()/add(), and just log the keypath directly. Note that this block:

keypathstr = ','.join(keypath)
self.logger.debug(f"Reading from [{keypathstr}]. Field = '{field}'")

logs the same thing as

self.logger.debug(f"Reading from {keypath}. Field = '{field}'")

Except it doesn't choke on None, and it makes the code simpler. We might as well fix all instances of this pattern.

The next issue is that calls to _search() require a keypathstr argument. However, this could be made an optional keyword argument that gets computed within _search() when it is first called, and then it can be passed along to each recursive call.

Along with building the keypathstr in _search(), we can also lower the the type-checking logic itself to this function, and not have repeated code across get()/getkeys().

Finally, we could then get rid of the similar blocks that already exist in set()/add(), which do this check but aren't actually fatal (meaning they don't prevent the problem in the first place).

[WRansohoff]

Good idea about using f'{keypath}' to avoid surfacing errors, and moving the key-checking into the shared _search method. I've made that change, and added a few set/get unit tests for these corner cases.

[nmoroze]

Great, thanks for addressing the comments and for adding the tests! Noticed a few last small things, but otherwise this looks good to go.

[aolofsson]

This is a great usability fix! I have resorted to putting print statements in the core code more than once to debug these None user errors, which clearly is not something that we want the end user to have to do.