[GitHub] [tvm] areusch commented on a diff in pull request #12521: [AutoTVM] Add support for text buffers to ApplyHistoryBest

[GitBox <gi...@apache.org>]

areusch commented on code in PR #12521:
URL: https://github.com/apache/tvm/pull/12521#discussion_r951879892


##########
python/tvm/autotvm/task/dispatcher.py:
##########
@@ -238,12 +246,12 @@ class ApplyHistoryBest(DispatchContext):
 
     Parameters
     ----------
-    records : str, list of str, or iterator of (autotvm.measure.MeasureInput,\
-                                                autotvm.measure.MeasureResult)
-        Collection of tuning records.
-        If is str, then it should be the filename of a records log file.
-        Each row of this file is an encoded record pair. If it is a list, it can either be
-        a list of paths to log files that will be loaded jointly or an iterator or records.
+    records : Records or iterator of Records objects, where a Records
+              object is a path-like object, a file-like object, or an
+              iterator of (MeasureInput, MeasureResult).
+
+        Collection of tuning records. If multiple Records objects are passed, their
+        contents will be merged.
     """
 
     def __init__(self, records):

Review Comment:
   should this arg get typed too?



##########
python/tvm/autotvm/task/dispatcher.py:
##########
@@ -30,18 +30,26 @@
 
 from __future__ import absolute_import as _abs
 
+from io import TextIOBase
 import logging
-import typing
-from typing import Union
-from collections.abc import Iterable
+from os import PathLike
+from pathlib import Path
+from typing import List, Iterable, Tuple, Union
 
 import numpy as np
 
 from .space import FallbackConfigEntity
 from .. import env as _env
+from ..measure import MeasureInput, MeasureResult
 
 logger = logging.getLogger("autotvm")
 
+Records = Union[
+    Union[str, bytes, Path],  # Path-like objects
+    TextIOBase,  # File-like objects
+    Iterable[Tuple[MeasureInput, MeasureResult]],

Review Comment:
   is this use case supported currently? i'm not sure whether we should support bypassing the load path here (this is technically "defensive programming" which i recognize is not great but i also think it might be prudent here).



##########
python/tvm/autotvm/task/dispatcher.py:
##########
@@ -458,11 +468,16 @@ def __init__(self, records):
             Otherwise, it is an iterator.
         """
         # pylint: disable=import-outside-toplevel
-        from ..record import load_from_file
+        from ..record import load_from_file, load_from_buffer
 
         super(ApplyGraphBest, self).__init__()
-        if isinstance(records, str):
+        if isinstance(records, str, bytes, PathLike):

Review Comment:
   i thought the types were supposed to be in a tuple, like:
   ```suggestion
           if isinstance(records, (str, bytes, PathLike)):
   ```



##########
python/tvm/autotvm/task/dispatcher.py:
##########
@@ -238,12 +246,12 @@ class ApplyHistoryBest(DispatchContext):
 
     Parameters
     ----------
-    records : str, list of str, or iterator of (autotvm.measure.MeasureInput,\
-                                                autotvm.measure.MeasureResult)
-        Collection of tuning records.
-        If is str, then it should be the filename of a records log file.
-        Each row of this file is an encoded record pair. If it is a list, it can either be
-        a list of paths to log files that will be loaded jointly or an iterator or records.
+    records : Records or iterator of Records objects, where a Records

Review Comment:
   i think it's reasonable to allow for the variety of ways to read in records, but just wondering if there's a use case for supporting `Iterator[Records]`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org