Open jbornschein opened 9 years ago
bartvm
commented
9 years ago Mm.. My difficulty with (2) is that if blocks-extras is not installed, but the user has a blocks-extras setting, it would still crash. I think that it's probably best to raise a warning but not crash, that way the user will be alerted, but we won't force them to delete potentially useful configuration just because Blocks can't find blocks-extras for whatever reason (e.g. PYTHONPATH is messed up).
nouiz
commented
9 years ago I don,t know how your config work, but Theano currently ignore config defined by the user, but that aren't defined as existing. Not the best.
One idea I had to make it better:
4) print when the process exit about such case. Doing it early is hard as we don't know the import order. As this shouldn't happen frequently, doing it late is at least better then not doing it.
Do you reuse Theano config system or you have your own?
On Mon, Jun 8, 2015 at 8:43 PM, Bart van Merriënboer < notifications@github.com> wrote:
Mm.. My difficulty with (2) is that if blocks-extras is not installed, but the user has a blocks-extras setting, it would still crash. I think that it's probably best to raise a warning but not crash, that way the user will be alerted, but we won't force them to delete potentially useful configuration just because Blocks can't find blocks-extras for whatever reason (e.g. PYTHONPATH is messed up).
mila-udem/blocks#712 https://github.com/mila-udem/blocks/pull/712
— Reply to this email directly or view it on GitHub https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110182993 .
bartvm
commented
9 years ago We have our own, more lightweight configuration parser (that uses YAML).
I think the best way is to raise a clear warning, but not crash. The problem with crashing is that if you check out an old version, update, etc. the same configuration file might suddenly cause a crash, which I can imagine must be frustrating if you're regularly switching between versions. On the other hand, you want to raise a warning so that a user finds out quickly if they made a typo in the configuration.
This is what I implemented for Blocks in https://github.com/mila-udem/blocks/pull/712
nouiz
commented
9 years ago I think your modif do the warning too early. But I didn't checked the detail. Is it the case that the config will get loaded before blocks.extras is loaded? If so, this would emit useless warning.
That is what would happen in Theano, maybe your import/config loading isn't in the same order.
On Tue, Jun 9, 2015 at 9:03 AM, Bart van Merriënboer < notifications@github.com> wrote:
We have our own, more lightweight configuration parser (that uses YAML).
I think the best way is to raise a clear warning, but not crash. The problem with crashing is that if you check out an old version, update, etc. the same configuration file might suddenly cause a crash, which I can imagine must be frustrating if you're regularly switching between versions. On the other hand, you want to raise a warning so that a user finds out quickly if they made a typo in the configuration.
This is what I implemented for Blocks in mila-udem/blocks#712 https://github.com/mila-udem/blocks/pull/712
— Reply to this email directly or view it on GitHub https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110350794 .
bartvm
commented
9 years ago No, you're right, it will still give a useless warning. The thing is, I'd like to avoid trying to import blocks.extras from inside blocks if at all possible.
I can't think of any way that would avoid unnecessary warnings then though. Alternative: Only raise a warning if an "unknown" configuration is found that is within a certain edit distance of a known one? On Jun 9, 2015 11:05 AM, "Frédéric Bastien" notifications@github.com wrote:
I think your modif do the warning too early. But I didn't checked the detail. Is it the case that the config will get loaded before blocks.extras is loaded? If so, this would emit useless warning.
That is what would happen in Theano, maybe your import/config loading isn't in the same order.
On Tue, Jun 9, 2015 at 9:03 AM, Bart van Merriënboer < notifications@github.com> wrote:
We have our own, more lightweight configuration parser (that uses YAML).
I think the best way is to raise a clear warning, but not crash. The problem with crashing is that if you check out an old version, update, etc. the same configuration file might suddenly cause a crash, which I can imagine must be frustrating if you're regularly switching between versions. On the other hand, you want to raise a warning so that a user finds out quickly if they made a typo in the configuration.
This is what I implemented for Blocks in mila-udem/blocks#712 https://github.com/mila-udem/blocks/pull/712
— Reply to this email directly or view it on GitHub < https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110350794
.
— Reply to this email directly or view it on GitHub https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110394782 .
nouiz
commented
9 years ago Do it when the process exit. This won't show the warning if the user import the module, it won't show up.
On Tue, Jun 9, 2015 at 11:21 AM, Bart van Merriënboer < notifications@github.com> wrote:
No, you're right, it will still give a useless warning. The thing is, I'd like to avoid trying to import blocks.extras from inside blocks if at all possible.
I can't think of any way that would avoid unnecessary warnings then though. Alternative: Only raise a warning if an "unknown" configuration is found that is within a certain edit distance of a known one? On Jun 9, 2015 11:05 AM, "Frédéric Bastien" notifications@github.com wrote:
I think your modif do the warning too early. But I didn't checked the detail. Is it the case that the config will get loaded before blocks.extras is loaded? If so, this would emit useless warning.
That is what would happen in Theano, maybe your import/config loading isn't in the same order.
On Tue, Jun 9, 2015 at 9:03 AM, Bart van Merriënboer < notifications@github.com> wrote:
We have our own, more lightweight configuration parser (that uses YAML).
I think the best way is to raise a clear warning, but not crash. The problem with crashing is that if you check out an old version, update, etc. the same configuration file might suddenly cause a crash, which I can imagine must be frustrating if you're regularly switching between versions. On the other hand, you want to raise a warning so that a user finds out quickly if they made a typo in the configuration.
This is what I implemented for Blocks in mila-udem/blocks#712 https://github.com/mila-udem/blocks/pull/712
— Reply to this email directly or view it on GitHub <
https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110350794
.
— Reply to this email directly or view it on GitHub < https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110394782
.
— Reply to this email directly or view it on GitHub https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110400810 .
bartvm
commented
9 years ago You mean using atexit? Isn't it a bit weird to give an error at the end basically saying "I've been ignoring this configuration all the time, so if things didn't work like expected, this is why"? On Jun 9, 2015 11:43 AM, "Frédéric Bastien" notifications@github.com wrote:
Do it when the process exit. This won't show the warning if the user import the module, it won't show up.
On Tue, Jun 9, 2015 at 11:21 AM, Bart van Merriënboer < notifications@github.com> wrote:
No, you're right, it will still give a useless warning. The thing is, I'd like to avoid trying to import blocks.extras from inside blocks if at all possible.
I can't think of any way that would avoid unnecessary warnings then though. Alternative: Only raise a warning if an "unknown" configuration is found that is within a certain edit distance of a known one? On Jun 9, 2015 11:05 AM, "Frédéric Bastien" notifications@github.com wrote:
I think your modif do the warning too early. But I didn't checked the detail. Is it the case that the config will get loaded before blocks.extras is loaded? If so, this would emit useless warning.
That is what would happen in Theano, maybe your import/config loading isn't in the same order.
On Tue, Jun 9, 2015 at 9:03 AM, Bart van Merriënboer < notifications@github.com> wrote:
We have our own, more lightweight configuration parser (that uses YAML).
I think the best way is to raise a clear warning, but not crash. The problem with crashing is that if you check out an old version, update, etc. the same configuration file might suddenly cause a crash, which I can imagine must be frustrating if you're regularly switching between versions. On the other hand, you want to raise a warning so that a user finds out quickly if they made a typo in the configuration.
This is what I implemented for Blocks in mila-udem/blocks#712 https://github.com/mila-udem/blocks/pull/712
— Reply to this email directly or view it on GitHub <
https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110350794
.
— Reply to this email directly or view it on GitHub <
https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110394782
.
— Reply to this email directly or view it on GitHub < https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110400810
.
— Reply to this email directly or view it on GitHub https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110407718 .
nouiz
commented
9 years ago Yes, but it is the best I found without giving extra warning. Otherwise, what about when compiling Theano function? Or other point in the software? Seem more complicated and hard to guess for users.
As this don't happen frequently a warning at the end is probably good enough I think.
On Tue, Jun 9, 2015 at 11:58 AM, Bart van Merriënboer < notifications@github.com> wrote:
You mean using atexit? Isn't it a bit weird to give an error at the end basically saying "I've been ignoring this configuration all the time, so if things didn't work like expected, this is why"? On Jun 9, 2015 11:43 AM, "Frédéric Bastien" notifications@github.com
wrote:
Do it when the process exit. This won't show the warning if the user import the module, it won't show up.
On Tue, Jun 9, 2015 at 11:21 AM, Bart van Merriënboer < notifications@github.com> wrote:
No, you're right, it will still give a useless warning. The thing is, I'd like to avoid trying to import blocks.extras from inside blocks if at all possible.
I can't think of any way that would avoid unnecessary warnings then though. Alternative: Only raise a warning if an "unknown" configuration is found that is within a certain edit distance of a known one? On Jun 9, 2015 11:05 AM, "Frédéric Bastien" notifications@github.com wrote:
I think your modif do the warning too early. But I didn't checked the detail. Is it the case that the config will get loaded before blocks.extras is loaded? If so, this would emit useless warning.
That is what would happen in Theano, maybe your import/config loading isn't in the same order.
On Tue, Jun 9, 2015 at 9:03 AM, Bart van Merriënboer < notifications@github.com> wrote:
We have our own, more lightweight configuration parser (that uses YAML).
I think the best way is to raise a clear warning, but not crash. The problem with crashing is that if you check out an old version, update, etc. the same configuration file might suddenly cause a crash, which I can imagine must be frustrating if you're regularly switching between versions. On the other hand, you want to raise a warning so that a user finds out quickly if they made a typo in the configuration.
This is what I implemented for Blocks in mila-udem/blocks#712 https://github.com/mila-udem/blocks/pull/712
— Reply to this email directly or view it on GitHub <
https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110350794
.
— Reply to this email directly or view it on GitHub <
https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110394782
.
— Reply to this email directly or view it on GitHub <
https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110400810
.
— Reply to this email directly or view it on GitHub < https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110407718
.
— Reply to this email directly or view it on GitHub https://github.com/mila-udem/blocks-extras/issues/8#issuecomment-110414184 .
If I understand correctly, this will never work given the current configuration infrastructure: https://github.com/mila-udem/blocks-extras/blob/master/blocks/extras/__init__.py because the blocks main library does not know about options defined there and will rightfully complain:
I see some obvious solutions -- all of them have unfortunately a certain
smell1) allow undeclared config options in .blocksrc to pass without error/warning. (-> Users with typos in their .blocksrc might hunt the bug for hours) 2) the main blocks/config.py could try to import blocks.extras.config.py and learn about additional options (-> I can allmost smell that circular dependency) 3) use a .blocks-extras-rc (-> total overkill and unintutive for such a basic functionalitly like plotting)From these I prefer (2) ... but maybe somebody has a better idea?