Emnlp #7
Emnlp #7
Conversation
(we should probably remove this later)
|
@chrisrytting @alexgshaw I think I've removed everything we wanted gone. |
|
You’re a git genius
…On Fri, May 19, 2023 at 6:12 PM Vin Howe ***@***.***> wrote:
@chrisrytting <https://github.com/chrisrytting> @alexgshaw
<https://github.com/alexgshaw> I think I've removed everything we wanted
gone.
—
Reply to this email directly, view it on GitHub
<#7 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALIJGOPMBEOL57ZMUYWPI4TXHAD5FANCNFSM6AAAAAAYIJFK5A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| # TODO: This is not a good way to do logging; we should actually use | ||
| # the logging module or something similar. | ||
| if self.logger: | ||
| self.logger.exception("Rate limited, retrying...") |
There was a problem hiding this comment.
logger.exception is only used to add context to caught exceptions—almost always in an except block. This isn't even really an error. logger.warning would probably be more appropriate here.
https://docs.python.org/3/library/logging.html#logging.Logger.exception
| # the logging module or something similar. | ||
| if self.logger: | ||
| self.logger.exception("Rate limited, retrying...") | ||
| print("Rate limited, retrying...", file=sys.stderr) |
There was a problem hiding this comment.
Everywhere I've seen, logging libraries replace regular print statements. Assuming that there is a logger available isn't an antipattern. Additionally, you could consider doing what projects like FastAPI do and define a global logger instead.
| print(e) | ||
| if self.logger: | ||
| self.logger.exception(e) |
There was a problem hiding this comment.
| ): | ||
| self.model_name = model_name | ||
| self.state_dict_path = state_dict_path | ||
| self.config_path = config_path | ||
| self.logger = logger |
There was a problem hiding this comment.
| if self.logger: | ||
| self.logger.exception(e) |
There was a problem hiding this comment.
| logger = logging.getLogger(__name__) | ||
| logger.setLevel(logging.ERROR) | ||
| handler = logging.FileHandler(Path(experiment_dir) / "errors.log") | ||
| handler.setLevel(logging.ERROR) | ||
| formatter = logging.Formatter("%(asctime)s %(message)s") | ||
| handler.setFormatter(formatter) | ||
| logger.addHandler(handler) | ||
| else: | ||
| logger = None |
There was a problem hiding this comment.
You can configure the global logger instead of passing a logger to every class. Passing a logger to every class feels a little bit like an antipattern.
|
|
||
| self._async_limiter = AsyncLimiter(OPENAI_RPM) | ||
|
|
||
| print(f"Using async {self.engine} engine.") |
There was a problem hiding this comment.
logger.info would be appropriate here
| @@ -50,24 +77,52 @@ def send_prompt(self, prompt, n_probs=100, **kwargs): | |||
| sorted_logprobs = dict( | |||
| sorted(logprobs.items(), key=lambda x: x[1], reverse=True) | |||
| ) | |||
| return sorted_logprobs | |||
| raise Exception("testing logging") | |||
There was a problem hiding this comment.
@chrisrytting this should definitely be removed
No description provided.