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

add requirements builder #434

Merged
merged 41 commits into from
Oct 25, 2022
Merged

add requirements builder #434

merged 41 commits into from
Oct 25, 2022

Conversation

madhur-tandon
Copy link
Contributor

@madhur-tandon madhur-tandon commented Oct 7, 2022

Closes #209

@madhur-tandon madhur-tandon requested a review from a team October 7, 2022 15:03
@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 15:03 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 18:51 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 19:05 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 19:20 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 19:43 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 19:56 Inactive
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 87.62% // Head: 87.73% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (0bd9007) compared to base (d60f3cf).
Patch coverage: 80.24% of modified lines in pull request are covered.

Additional details and impacted files
@@                Coverage Diff                @@
##           release/0.3.0     #434      +/-   ##
=================================================
+ Coverage          87.62%   87.73%   +0.10%     
=================================================
  Files                 94       96       +2     
  Lines               7847     7882      +35     
=================================================
+ Hits                6876     6915      +39     
+ Misses               971      967       -4     
Impacted Files Coverage Δ
mlem/api/__init__.py 100.00% <ø> (ø)
mlem/cli/apply.py 94.23% <ø> (ø)
mlem/cli/build.py 100.00% <ø> (ø)
mlem/cli/checkenv.py 100.00% <ø> (ø)
mlem/cli/clone.py 100.00% <ø> (ø)
mlem/cli/dev.py 50.00% <ø> (ø)
mlem/cli/import_object.py 100.00% <ø> (ø)
mlem/cli/init.py 100.00% <ø> (ø)
mlem/cli/link.py 100.00% <ø> (ø)
mlem/cli/serve.py 89.47% <ø> (ø)
... and 70 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 20:43 Inactive
@madhur-tandon madhur-tandon self-assigned this Oct 7, 2022
@madhur-tandon madhur-tandon temporarily deployed to internal October 7, 2022 20:53 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 8, 2022 11:11 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 8, 2022 11:20 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 8, 2022 11:44 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 8, 2022 12:10 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 8, 2022 12:31 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 9, 2022 05:43 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 9, 2022 05:47 Inactive
@aguschin
Copy link
Contributor

Re the issue you posted about pipenv - the issue was closed in 2020, so it's a bit outdated. If you check, they are releasing quite frequently now. So don't think the project is dead, unless I'm missing something.

@madhur-tandon
Copy link
Contributor Author

Re the issue you posted about pipenv - the issue was closed in 2020, so it's a bit outdated. If you check, they are releasing quite frequently now. So don't think the project is dead, unless I'm missing something.

You can check numerous posts on the internet:

@aguschin
Copy link
Contributor

aguschin commented Oct 10, 2022

Ok, what I read in your links:

  1. some people think pipenv is not useful - well, there are people who like and use pipenv as well, you can check it in used by: 39k here
  2. pipenv have some problems and there are workarounds and troubleshooting guide

I would like to note that I'm not insisting on implementing something in particular (like pipenv, poetry or conda). The only thing that is must be implemented is obviously virtualenv. The others are optional to my mind. But I don't see reasons to reject pipenv implementation completely.

I personally would consider implementing virtualenv only for now to get some feedback before we go into advanced tools like conda, pipenv, poetry or something else.

@madhur-tandon
Copy link
Contributor Author

I personally would consider implementing virtualenv only for now to get some feedback before we go into advanced tools like conda, pipenv, poetry or something else.

I have done virtualenv and conda as of now in this PR

mlem/contrib/venv.py Outdated Show resolved Hide resolved
mlem/contrib/venv.py Outdated Show resolved Hide resolved
mlem/contrib/venv.py Outdated Show resolved Hide resolved
mlem/contrib/venv.py Outdated Show resolved Hide resolved
mlem/contrib/venv.py Outdated Show resolved Hide resolved
@madhur-tandon madhur-tandon temporarily deployed to internal October 18, 2022 11:03 Inactive
@madhur-tandon madhur-tandon changed the base branch from main to release/0.3.0 October 18, 2022 11:03
@madhur-tandon madhur-tandon temporarily deployed to internal October 18, 2022 11:25 Inactive
@madhur-tandon madhur-tandon temporarily deployed to internal October 18, 2022 11:39 Inactive
reqs = obj.requirements.of_type(req_type_cls)
if self.target is None:
reqs_representation = [r.get_repr() for r in reqs]
requirement_string = "\n".join(reqs_representation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe " ".join? Will it work like pip install $(mlem build requirements -m model) RN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have replaced it with " ", but pip install $(mlem build requirements -m model) doesn't work because of the first line with the emoji i.e. ⏳️ Loading model from sk-model.mlem.

Is there a way to ignore all this echo related stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question! This is a good example when we should try our tools in a user scenario to find out how it will work in fact :)

This is what I heard multiple times from Dmitry and Ivan, btw. See this #390 Let's discuss it there, so it won't get lost.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add --quiet/-q to mlem-callback that will disable standard cli output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this to be done? As a separate PR maybe? Or I need to add this --quiet stuff somewhere? I do see stuff related to mlem_group_callback so wondering if it is to be accommodated there?

Also, should this PR be merged since it's approved OR should we do the above first?

mlem/core/requirements.py Outdated Show resolved Hide resolved
tests/contrib/test_venv.py Outdated Show resolved Hide resolved
tests/contrib/test_venv.py Outdated Show resolved Hide resolved
tests/contrib/test_venv.py Outdated Show resolved Hide resolved
@madhur-tandon madhur-tandon merged commit d1f653a into release/0.3.0 Oct 25, 2022
@madhur-tandon madhur-tandon deleted the feature/env-builder branch October 25, 2022 10:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Setting up venv from model.mlem meta
3 participants