summon: fixes and dvcx prereq#3101
Conversation
efiop
left a comment
efiop
left a comment
There was a problem hiding this comment.
Check the deep source report. Looks like you are intriducing the same issue that you've reported recently in the private converstaions 🙂 The open one, but now with summon.
| for out in outs: | ||
| repo.cloud.pull(out.get_used_cache()) | ||
| out.checkout() | ||
| raise SummonError(str(exc)) from exc |
There was a problem hiding this comment.
I guess users of summon are not going to set up the DVC logger, right?
These messages would duplicate the output (exc - exc).
@efiop introduced an extra keyword to prevent that: extra={"tb_only": True}, if it is the case, maybe you could use it.
There was a problem hiding this comment.
The cause will have the same message as SummonError but different traceback, I don't think this is an issue. I can't use extra={"tb_only": True} because I am not using logger.exception() but simply raise an exception.
There was a problem hiding this comment.
@efiop because a user won't see the issue until he/she scrolls up past traceback. "invalid formatting" is useless, you need to see, which key is absent/extraneous/has wrong type.
There was a problem hiding this comment.
Missed that it is not meant to go through our usual logger logic. I'm fine with it 👍
It's now thread-safe, but the rest is still relevant.
- proper doc-string for prepare_summon() - summon -> summon_dict in summon() to not overwrite a glo bal name. This was not a bug, but a bad practice, which might have caused some issues in the future. - better name for _get_object_spec()
|
Should be mergable now. |
2 participants
No description provided.