Skip to content
This repository has been archived by the owner on Aug 10, 2024. It is now read-only.

requirement, venv, conda builder docs #191

Merged
merged 17 commits into from
Oct 27, 2022
Merged

requirement, venv, conda builder docs #191

merged 17 commits into from
Oct 27, 2022

Conversation

madhur-tandon
Copy link
Contributor

No description provided.

@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 13, 2022 08:41 Inactive
@madhur-tandon madhur-tandon changed the base branch from main to new-docs-structure October 13, 2022 08:42
@github-actions
Copy link

github-actions bot commented Oct 13, 2022 •
edited
Loading

2d34fec

Link Check Report

3/180 links failed.

CML watermark

@madhur-tandon madhur-tandon mentioned this pull request Oct 13, 2022
Merged
@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 13, 2022 08:55 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 13, 2022 09:04 Inactive
@madhur-tandon madhur-tandon self-assigned this Oct 13, 2022
@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 13, 2022 09:55 Inactive
aguschin
aguschin reviewed Oct 13, 2022
View reviewed changes
content/docs/sidebar.json Outdated Show resolved Hide resolved
aguschin
aguschin reviewed Oct 13, 2022
View reviewed changes
Copy link
Contributor

@aguschin aguschin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done some review, please extend comments to other pages in this PR when applicable :)

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Oct 16, 2022 •
edited
Loading

9/115 links failed.

☝🏼 PTAL

U: 3 links are still failing @madhur-tandon 🙏🏼

@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 21, 2022 08:32 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 25, 2022 11:39 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 26, 2022 12:44 Inactive
aguschin
aguschin approved these changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@aguschin aguschin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's address few questions left and merge!

jorgeorpinel
jorgeorpinel reviewed Oct 27, 2022
View reviewed changes
content/docs/user-guide/building/venv.md
Comment on lines +1 to +8
# Virtual Environments

Given a model and a list of its dependencies and packages, an environment needs
to be present that has these requirements readily available so as to use the
model. To make sure that different dependencies for different models (or
projects) don't clash,
[virtual environments](https://realpython.com/python-virtual-environments-a-primer/)
are used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be a section in UG/ Building/ Requirements ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link from somewhere in Python Packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be a section in UG/ Building/ Requirements ?

The Description section already has a link to UG / Requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link from somewhere in Python Packages?

not sure if this is related. Because the guide linked above is to create a python package FOR an mlem model, and not to install python packages REQUIRED by an mlem model.

jorgeorpinel
jorgeorpinel reviewed Oct 27, 2022
View reviewed changes
content/docs/user-guide/building/requirements.md
Comment on lines +68 to +72
## Custom requirements

Custom requirements represent local python code such as files, zipped sources,
etc. Custom requirements always need the `--target` option since they are
materialized at the target.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link from somewhere in Python Packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if these are related again.

@jorgeorpinel
Copy link
Contributor

I think I've left enough feedback for this one. Looking better! Leaving it up to you guys.

@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 27, 2022 08:34 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 27, 2022 08:37 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 27, 2022 08:55 Inactive
@madhur-tandon
Copy link
Contributor Author

U: 3 links are still failing @madhur-tandon 🙏🏼

This is expected for localhost:8000 since that will always fail. Should I remove the linking?

And for the other two, they will fix themselves when release/0.3 branch is merged to main for mlem repository.

@shcheklein shcheklein temporarily deployed to mlem-ai-requirement-bui-vdivzc October 27, 2022 09:15 Inactive
@madhur-tandon madhur-tandon merged commit d39200e into new-docs-structure Oct 27, 2022
@madhur-tandon madhur-tandon deleted the requirement-builder-docs branch October 27, 2022 09:21
@jorgeorpinel
Copy link
Contributor

Thanks @madhur-tandon . I see this is merged to the parent branch only so I can still reply re localhost:8000: that should probably never get to the code right?

@aguschin
Copy link
Contributor

aguschin commented Oct 28, 2022 •
edited
Loading

No, localhost link should be in code. Idea is that right before that link you run mlem serve, that spins up a server there. So even that it will fail all the time in our CI tests here, it should be there. If there is a way to disable the check for that specific link, please share that.

@jorgeorpinel
Copy link
Contributor

I don't see localhost in https://github.com/iterative/mlem.ai/pull/191/files 🤷🏼

@jorgeorpinel jorgeorpinel mentioned this pull request Oct 29, 2022
Merged
16 tasks
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) p1-important Active priorities to deal within next sprints C: user-guide Content of /doc/user-guide C: get-started Content of /doc/get-started C: ref Content of /doc/{command,api,object}-reference labels Oct 30, 2022
@aguschin
Copy link
Contributor

It's here https://mlem.ai/doc/get-started/serving#making-requests @jorgeorpinel

@jorgeorpinel
Copy link
Contributor

Should not be a real link IMO.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jorgeorpinel jorgeorpinel jorgeorpinel left review comments

@aguschin aguschin aguschin approved these changes

@mike0sv mike0sv Awaiting requested review from mike0sv

Assignees

@madhur-tandon madhur-tandon

Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: get-started Content of /doc/get-started C: ref Content of /doc/{command,api,object}-reference C: user-guide Content of /doc/user-guide p1-important Active priorities to deal within next sprints
Projects
No open projects
MLEM + GTO
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@madhur-tandon @jorgeorpinel @aguschin @mike0sv @shcheklein