Skip to content
This repository has been archived by the owner on Nov 26, 2022. It is now read-only.

Allow users to disable the warning and sleep delay when running an algorithm #493

Closed
samueldg opened this issue Oct 12, 2018 · 1 comment

Comments

@samueldg
Copy link
Contributor

samueldg commented Oct 12, 2018

While looking into some performance bottlenecks in Catalyst, I stumbled onto this line of code: https://github.com/enigmampc/catalyst/blob/90d9e4e2def79b49c32607ce7da349598a117021/catalyst/utils/run_algo.py#L151
which seemed unnecessary.

Cf #283 , and PR #421

However after digging, I found the commit that introduces it (55d1fee), and it does make sense. I understand, and fully agree with, the rationale of warning users that this is in alpha state.

I have a suggestion that would tick all the boxes IMO:

  • satisfies maintainers, who want to warn users of the alpha status of the project;
  • satisfies "power users", who would like to dismiss/disable the warning, which they are obviously OK with;
  • doesn't require users manually patching their local copy of Catalyst;
  • doesn't change the current default behaviour;
  • really simple to implement.

The solution is to have the ability to disable the warning and sleep delay with an environment variable (e.g. $CATALYST_DISABLE_ALPHA_WARNING).

This means the following check:

log.info('Catalyst version {}'.format(catalyst.__version__))
if not os.environ.get('CATALYST_DISABLE_ALPHA_WARNING'):
    log.warn(ALPHA_WARNING_MESSAGE)
    sleep(3)

This option doesn't even need to be documented, if you feel it might have bad consequences, but at least people stumbling on that code/issue can set the env variable themselves and call it a day!

If you're interested in this approach, I'll gladly submit a pull request!

@lenak25
Copy link
Contributor

lenak25 commented Oct 14, 2018

@samueldg ,

Thanks for your idea! It indeed makes sense and might help other users that feel the same.
It will be great if you could submit a PR, which we can than merge and release a new version including both of your performance improvements (together with #494 ).

Thanks,

Lena

samueldg added a commit to samueldg/catalyst that referenced this issue Oct 15, 2018
By setting the `$DISABLE_ALPHA_WARNING` environment variable to
anything else than an empty string, users can now disable
the warning about Catalyst being in alpha, and the associated three
seconds delay.

Fixes scrtlabs#493
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants