Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Get started feedback #685

Open
19 tasks
dmpetrov opened this issue Jul 2, 2023 · 0 comments
Open
19 tasks

Get started feedback #685

dmpetrov opened this issue Jul 2, 2023 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@dmpetrov
Copy link
Member

dmpetrov commented Jul 2, 2023

  • Not easy to read selected code:
Screenshot 2023-07-01 at 6 42 54 PM
  • A bug in the tutorial. I copy-paste it step by step.
>>> save(rf, "models/rf", sample_data=df)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'df' is not defined. Did you mean: 'rf'?
  • Why is it called data? Shouldn't it be model? If the type is unknown - something else.
artifacts:
  data:
    hash: 6bd620f3b5e5bbe35fef22ab51d49b34
  • save(rf, "mlem-m", sample_data=data) returns (and prints in iPython) a massive object. It does not look good. Wouldn't it be better to suppress printing? Like model = save(...)
Screenshot 2023-07-01 at 6 51 40 PM
  • In model.mlem - is there any chance to make a single list of params instead of two lists of names and types?
    methods:
      predict:
        args:
        - name: X
          type_:
            columns:
            - sepal length (cm)
            - sepal width (cm)
            - petal length (cm)
            - petal width (cm)
            dtypes:
            - float64
            - float64
            - float64
            - float64
            index_cols: []
            type: dataframe
  • name: predict - isn't it duplication with predict from the above:
    methods:
      predict:    # <--- this predict
        args:
        - name: X
          type_:
         # ....
        name: predict # <--- isn't it the same predict?
  • Is underscore really needed in type_?
  • Can be null just skipped by default?
          shape:
          - null
  • Another workflow bug - pd were never imported
>>> df = pd.DataFrame([[0, 1, 2, 3]], columns=features)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'pd' is not defined. Did you mean: 'id'?
  • Workflow missmatch - first scrips were considered interactive (train.py and saving models script with no name) while predict.py is running from CLI. It confuses user. Is it possible to make it fully interactive and working end-to-end? Clarification - it's ok to run CLI command in special cases like running mlem commands but not python.

  • Making requests section - a curl command example can be a better rather than a link to Swagger UI

  • Docker deployment bug:

 $ mlem deployment run docker_container docker_app.mlem \
>     --model models/rf \
>     --image_name mlem-rf-server \
>     --server fastapi
Usage: mlem deployment run docker_container  path
Try 'mlem deployment run docker_container -h' for help.
╭─ Error ───────────────────────────────────────────────────────────────────────────────────╮
│ No such option: --image_name (Possible options: --container_name, --image.name,           │
│ --image.tag)                                                                              │
╰───────────────────────────────────────────────────────────────────────────────────────────╯
  • Docker deployment error formatting - error output is not standard for CLI. The value of the frame is not clear but it takes quite a bit of space.
╭─ Error ───────────────────────────────────────────────────────────────────────────────────╮
│ No such option: --image_name (Possible options: --container_name, --image.name,           │
│ --image.tag)                                                                              │
╰───────────────────────────────────────────────────────────────────────────────────────────╯
  • --image.name, --image.tag etc - not standard CLI notation for parameters. It has to be --image-name

  • Prefix-Emoji in error messages and warning are not a good practice in CLI - ❌ Error calling docker:.

  • It is saying that docker is running but there is nothing in https://localhost:8080/docs Bug?

$ mlem deployment run docker_container docker_app.mlem --model mlem-m  --image.name mlem-rf-server     --server fastapi
⏳️ Loading deployment from docker_app.mlem
⏳️ Loading model from mlem-m.mlem
🛠 Creating docker image mlem-rf-server
  💼 Adding model files...
  🛠 Generating dockerfile...
  💼 Adding sources...
  💼 Generating requirements file...
  🛠 Building docker image mlem-rf-server:latest...
  ✅  Built docker image mlem-rf-server:latest
✅  Container mlem-deploy-1688264880 is up
  • Does the full path really needed? The full path is not secured and makes state untransferable to other dir or machine. Can a relative path be used?
$ cat docker_app.mlem.state
...
model_link:
  path: /Users/dmitry/src/mm/mlem-m.mlem
  • (Not from the tutorial) Batch size changes the type of output. It is a bug.
$ mlem apply mlem-m d1-m --output ooo --json
...
>>> type(load('ooo'))
<class 'numpy.ndarray'>


$ mlem apply mlem-m d1-m --output o1 --json --batch_size 2
...
>>> type(load('o1'))
<class 'list'>
  • (Not from the tutorial) I'm not sure I understand --json option, it does not change the output. See example above.
@dmpetrov dmpetrov added the bug Something isn't working label Jul 2, 2023
@aguschin aguschin self-assigned this Jul 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants