WIP: Error messages #348
WIP: Error messages #348
Conversation
- shortens code - raises CircularDependencyError instead of RecursionError when there is a circular depencency
|
Oh, I just noticed that I used some 3.6 syntax. That must be removed. |
|
Hey! This looks amazing. Thanks a lot for all the effort! For the "did you mean" suggestions: |
| for ingred in self.ingredients: | ||
| for cmd_name, cmd in ingred.gather_commands(): | ||
| yield cmd_name, cmd | ||
| def _gather(self, func): |
There was a problem hiding this comment.
I like the idea of a generic gather function a lot. This removes some of the code duplication and as you said, it raises the more descriptive CircularDependencyError.
Some minor aesthetics: the usage of this function with the inline lambda expressions is a bit unwieldy. How about using it as a decorator? Something like this:
@self._gather_from_ingredients
@staticmethod
def gather_commands(ingredient):
for command_name, command in ingredient.commands.items():
yield join_paths(ingredient.path, command_name), commandNot entirely sure if that works, because of the interaction with @staticmethod, but it feels cleaner and more readable. Alternatively having a separate get_commands method that is used inside the gather_commands method instead of the lambda expression could be good too.
There was a problem hiding this comment.
I like the idea of the decorator. self is unfortunately not defined outside of the function. We can define a decorating function outside of the class body:
def gather_from_ingredients(f):
def wrapped(self):
for ingredient, _ in self.traverse_ingredients():
for item in f(ingredient):
yield item
return wrapped
class Ingredient(object):
# ...
@gather_from_ingredients
def gather_commands(ingredient):
for command_name, command in ingredient.commands.items():
yield join_paths(ingredient.path, command_name), command
| for ingredient, _ in self.traverse_ingredients(): | ||
| for name, item in func(ingredient): | ||
| if ingredient == self: | ||
| name = name[len(self.path) + 1:] |
There was a problem hiding this comment.
Not sure I understand: what is the purpose of this if clause?
There was a problem hiding this comment.
The command names of the experiment itself should not be prefixed with the experiment name. traverse_ingredients returns all ingredients and the gathering function returns the full names. I kept the previous behavior by removing the own path prefix from the name of the experiment.
| for ingredient in self.ingredients: | ||
| for ingred, depth in ingredient.traverse_ingredients(): | ||
| yield ingred, depth + 1 | ||
| except CircularDependencyError as e: |
There was a problem hiding this comment.
You use this pattern a lot. Basically:
try:
# stuff
except SomeCustomError as e:
# special handling (i.e. adding information) of that errorI think it might be nicer to provide a context manager to provide that special handling without cluttering the code too much. It could even be part of the exception itself. Something like:
with CircularDependencyError.track(self): # not sure about the name
# stuffShouldn't be hard to implement and would improve readability IMHO.
There was a problem hiding this comment.
I already thought about doing this. This should be easy to implement and improve readability a lot.
| @@ -0,0 +1,223 @@ | |||
| import inspect | |||
There was a problem hiding this comment.
Having a separate exception.py file is a good idea. But I think you forgot to remove the Exceptions from utils.py ;-)
There was a problem hiding this comment.
Oh, I didn't want to push ´exception.py´. At the moment, the exceptions depend on ´utils.py´, and moving the exceptions without an import in ´utils.py´ would be incompatible to the current version.
|
|
||
| from sacred.config.config_sources import ConfigSource | ||
|
|
||
| if colored_exception_output: |
There was a problem hiding this comment.
This variable is not defined anywhere. You import it from utils later, but also there it is not defined.
Also, you probably should import BLUE, GREEN, etc. in either case. ;-)
Should this maybe be in settings? Maybe alongside the colors? Or did you plan on auto-detecting if the console supports color? In that case it might be worth it to have a separate colored_output file that takes care of that logic and defines the colors. Aren't there some good libraries to handle this?
| raise | ||
| except Exception as e: | ||
| if not self.current_run or not self.current_run.debug \ | ||
| or not self.current_run.pdb: |
There was a problem hiding this comment.
Confusing if-statement. Are you sure this is what you want? The only case in which this will not execute is if there is a current_run and debug is true and pdb is true,
| set_by_dotted_path(config_updates, | ||
| join_paths(scaff.path, ncfg_key), | ||
| value) | ||
|
|
||
| distribute_config_updates(prefixes, scaffolding, config_updates) | ||
|
|
||
| distribute_config_sources(prefixes, scaffolding, config_sources) | ||
|
|
There was a problem hiding this comment.
Wow, this piece of code got even more complicated. If this work as intendend, I am impressed. Even before your additions I was having difficulties mentally keeping track of what happens. This is not good (but not your fault). I think a serious refactoring might be needed here.
If I understand correctly, you are capturing where a particular config entry originated from and passing this information around. Maybe there is a way to somehow encapsulate this into the config entries inside the configuration process. I'll have to think about it. (This is just me thinking out loud, not an instruction to you).
There was a problem hiding this comment.
This piece of code is very ugly and I'm not sure if it works in all cases. It worked for all scenarios I tested, but it may fail for others. I think I should rethink and rewrite this part, including tests.
There was a problem hiding this comment.
One (again very rough) idea to make it more structured and clear (I may be missing some important points and maybe it is not possible like this):
We could add these configuration origins to the ConfigSummary class and use the ConfigSummary throughout the whole configuration process, and not just at the end to track the changes. This would e.g. simplify the interface of chain_evaluate_config_scopes to just return a ConfigSummary and we could combine this with the config updates using a simple ConfigSummary.update_from (or similar). The config sources for config scopes could then be added inside the config scopes, and the config scope's __call__ would also return a ConfigSummary. We could store everything hierarchically (so, every sub-dict would be a ConfigSummary, same for lists) and create an empty structure based on the ingredient hierarchy before starting the configuration process to get rid of these calls to distribute_config_updates, because we could directly write to the sub-ConfigSummary of the scaffolding using ´recursive_update´ or set_by_dotted_path.
|
Hi, I left a few thought in the code, but let me also comment on a high level:
In general: this is a very large PR, which will make it hard to test and review. That is not a show-stopper, but if you can split it into smaller chunks, I think it will get into |
|
Thanks for your feedback! I agree that this is a very large PR and splitting it into smaller ones seems appropriate. This is a first rough suggestion of how to split it:
|
|
Great! 🎉 Oxt weekend I might be able to spend a day on refactoring Does that sound good to you? Any further thoughts? |
|
Yes that sounds great! I'm glad that you like the idea to use ConfigSummaries. My suggested "hierarchical structure" that can be updated by recursive dict updates might be a lot more complicated than I thought, especially when the same Ingredient appears in different places. But I think you have a lot better overview over sacred and the configuration process and maybe come up with better ideas. I first had to check what "oxt weekend" means, but I find that a great idea. I am always confused by the phrase "next weekend"... I'll wait until you made some progress (hopefully oxt weekend) and then start to implement the "tracking of origin of config values" feature based on your changes. What do you think about colored output for the error messages, e.g. highlight all config keys with the same color? |
|
Hey @thequilo. If you are interested in the current state you can find the code in the Some highlights of what I am trying to do:
Regarding colored output messages: I think that is a good idea and there are probably several opportunities for helpful highlighting. Since this concerns error messages, we might need to be careful about not breaking the output in terminals that don't support color (Though, I am not sure if that is even a real problem). |
|
It looks like you already made some progress that looks promising. I like the changes that you are trying to do. I'll just wait with my modifications until you finished implementing it. I found that the |
|
First, happy new year! How's the progress on the |
|
Hi @thequilo, |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
unstale |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Unstale: This work is still not finished yet, but waiting for the config refactoring to finish |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Still waiting for config v2 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Still waiting |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This is a WIP pullrequest that addresses the error messages as described in #239. I finally got time to prepare a pullrequest for this.
The code is currently very ugly and could more be seen as a proof of concept that those things can work.
There are lists of things that are already done and that need to be done below.
Any suggestions for further improvement of the error messages (or the code, since some part of it is not well structured and untested) are welcome.
Done
Use iterate_ingredients for gathering commands and named configs
This causes
gather_commandsandgather_named_configsto raise aCircularDependencyErrorinstead of aRecursionError, which makes much clearer what is causing the error.In addition, any future
gather_somethingfunctions that may be implemented can overwrite one method and the error handling is done initerate_ingredients, and the path filtering for experiments is done there.Track Ingredients that cause circular dependencies
The
CircularDependencyErroris caught initerate_ingredientsand the current ingredient is added to a listCircularDependencyError.__ingredients__to keep track of which ingrediens cuased the circular depenceny.An example error:
Track sources of configuration entries
This code is still very ugly, but it allows to track the sources of configuration values.
This works up to different resolutions:
ConfigScope, we can find the wrapped function and get the place of definition of this function (file + line of the signature line)inspect.stackto find the line in which the dict configuration value was added.See the
InvalidConfigErrorfor examples.Add a baseclass
SacredErrorfor future Excpetions that is pretty printed inexperiment.run_commandlineThe init definition looks like this:
It provides the following additional arguments (that are handled in
experiment.run_commandline):print_traceback: ifTrue, traceback is printed according tofilter_traceback. IfFalse, no traceback is printed (except for the Exception itself)filter_traceback: IfTrue, the traceback is filtered (WITHOUT sacred internals), ifFalse, it is not filtered and ifNone, it falls back to the previous behaviour (filter if not raised within sacred)print_usage: The short usage is printed when this is set toTrue.Add an
InvalidConfigErrorthat can be raised in user codeAdded an
InvalidConfigErrorthat prints the conflicting configuration values.Example:
MissingConfigError
Prints missing configuration values. Prints the filtered stack trace by default, so that the function call that is missing values can be found.
It also prints the name of the ingredient that captured the function and the file in which the captured function is defined.
Example error:
NamedConfigNotFoundError
Raise a
NamedConfigNotFoundErrorinstead ofKeyError, and don't print traceback.TODO
ConfigAddedError
Raise a
ConfigAddedErrorwhen a config value is added that is not used anywhere. This is a sublcass ofConfigErrorand prints the source where the new configuration value is defined:TODO
TODO
ConfigAddedError