Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package without editable flag #576

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

VukW
Copy link
Contributor

@VukW VukW commented Apr 10, 2024

An ancestor of #550 (branch was renamed)

The goal is to make medperf client installable from pypi. This PR is the first part that allows installing package not from repository

A few changes were implemented:

  • git branch check updates: check skipped if no repo found.
  • During deployment, local server places cert.crt, cert.key and mock_tokens/tokens.json to ~/.medperf_config/.local_server/ so detached medperf client can use it. NB: this change breaks the behavior for existing developers (as path to local server cert is already defined in user config). You should update local config's certificate path to ~/.medperf_config/.local_server/cert.crt.
  • pip install -e ./cli replaced with pip install ./cli everywhere

@VukW VukW requested a review from a team as a code owner April 10, 2024 12:20
@VukW VukW had a problem deploying to testing-external-code April 10, 2024 12:21 — with GitHub Actions Failure
Copy link
Contributor

github-actions bot commented Apr 10, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@VukW VukW had a problem deploying to testing-external-code April 10, 2024 12:36 — with GitHub Actions Failure
@VukW VukW had a problem deploying to testing-external-code April 23, 2024 13:28 — with GitHub Actions Failure
aristizabal95
aristizabal95 previously approved these changes Apr 26, 2024
Copy link
Contributor

@aristizabal95 aristizabal95 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@VukW VukW had a problem deploying to testing-external-code April 29, 2024 13:50 — with GitHub Actions Failure
@VukW VukW force-pushed the package-without-editable-flag branch from 49dc914 to 872cc66 Compare April 29, 2024 14:07
@VukW VukW temporarily deployed to testing-external-code April 29, 2024 14:07 — with GitHub Actions Inactive
@hasan7n hasan7n self-requested a review May 3, 2024 14:30
Copy link
Contributor

@hasan7n hasan7n left a comment

Choose a reason for hiding this comment

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

@VukW looks good, although I believe there is no need to move around tokens and certs since we decided that developers will anyway clone the repo and install it from source, so the added complexity of moving mock tokens file and having a ~ medperf dev folder could be really omitted; nothing is gained I guess?
anyway please feel free to merge or edit

@hasan7n
Copy link
Contributor

hasan7n commented May 3, 2024

@VukW ah I guess the auth test needs to have --cert .. removed from calling seed

@VukW VukW had a problem deploying to testing-external-code May 6, 2024 15:07 — with GitHub Actions Failure
@VukW
Copy link
Contributor Author

VukW commented May 7, 2024

@VukW looks good, although I believe there is no need to move around tokens and certs since we decided that developers will anyway clone the repo and install it from source, so the added complexity of moving mock tokens file and having a ~ medperf dev folder could be really omitted; nothing is gained I guess? anyway please feel free to merge or edit

Looked one more time to it.
So we have 2 issues (location of local cert + location of local tokens).

local tokens

Local tokens path is hardcoded in config.py. We can:

  1. move it to config.yaml to make it configurable (but we either have to declare it in every profile including default, or make a architectural difference between default profile and local); + every time you recreate config you'd have to remember you need to fix this field manually
  2. require path to be passed via env variable (but then you'd have to provide the env var for all your shells - say, customize your ~/.bashrc or zsh/fish profile) (AFAIR your proposed this, right?)
  3. keep as implemented now, cluttering home folder with ~/.medperf_dev/`

IMO it's better to do (2) or (3), as (1) looks more error-prone

Local cert

It's already defined in config.yaml. So two solutions are

  1. fill this field manually every time you recreate config
  2. keep as implemented now, cluttering home folder with ~/.medperf_dev

To tell truth I'm confused and not sure which solution I'd prefer as all of them seem to have some issues. But, maybe, it's easier to me to agree with cluttering home folder rather then fix config fields manually after any config reset (??? tho I believe it would not be really often).

Need your opinion, man @hasan7n :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants