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.

Sign up for GitHub

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

Jump to bottom

chore: Remove model hub #9869

Merged
merged 10 commits into from
Sep 6, 2024
Merged

chore: Remove model hub #9869

merged 10 commits into from
Sep 6, 2024

Conversation

MikhailKardash
Copy link
Contributor

@MikhailKardash MikhailKardash commented Aug 26, 2024 •
edited by jira bot
Loading

Ticket

https://hpe-aiatscale.atlassian.net/browse/MD-461

Description

Remove model_hub library and subdirectory from determined

Test Plan

No testing required

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@cla-bot cla-bot bot added the cla-signed label Aug 26, 2024
@determined-ci determined-ci requested a review from a team August 26, 2024 23:01
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

1 similar comment
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci determined-ci added the documentation Improvements or additions to documentation label Aug 26, 2024
@netlify Netlify
Copy link

netlify bot commented Aug 26, 2024 •
edited
Loading

✅ Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 4e56b56
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66d0b02880a0690008d6e711

@codecov Codecov
Copy link

codecov bot commented Aug 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.58%. Comparing base (a1234a1) to head (4e56b56).
Report is 25 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9869      +/-   ##
==========================================
- Coverage   54.66%   54.58%   -0.09%     
==========================================
  Files        1261     1247      -14     
  Lines      156328   155531     -797     
  Branches     3584     3584              
==========================================
- Hits        85463    84898     -565     
+ Misses      70733    70501     -232     
  Partials      132      132
Flag Coverage Δ
backend 45.21% <ø> (-0.07%) ⬇️
harness 72.78% <ø> (+0.16%) ⬆️
web 54.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 4 files with indirect coverage changes

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@MikhailKardash MikhailKardash marked this pull request as ready for review August 26, 2024 23:26
@MikhailKardash MikhailKardash requested a review from a team as a code owner August 26, 2024 23:26
dannysauer
dannysauer approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@dannysauer dannysauer left a comment

Choose a reason for hiding this comment

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

LGTM. A couple of optional grumbles to prove I read the whole PR.

.circleci/real_config.yml Outdated Show resolved Hide resolved
.circleci/real_config.yml Outdated Show resolved Hide resolved
docs/.redirects/redirects.json
Comment on lines +23 to +24
"setup-cluster/deploy-cluster/slurm/hpc-with-agent": "../../slurm/_index.html",
"setup-cluster/slurm/hpc-with-agent": "_index.html",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related, right? ;)

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 had to fix some redirects using docs/redirects.py. This is auto-generated.

docs/.redirects/redirects.json
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding missing newlines back.

/Grumbles at whatever web editor some people use which keeps pruning one byte from files.

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

1 similar comment
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@MikhailKardash MikhailKardash requested a review from caehd10 August 27, 2024 17:17
tara-hpe
tara-hpe approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@tara-hpe tara-hpe left a comment

Choose a reason for hiding this comment

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

LGTM

jgongd
jgongd reviewed Aug 27, 2024
View reviewed changes
Copy link
Contributor

@jgongd jgongd left a comment

Choose a reason for hiding this comment

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

Other places that have model hub we may want to remove:

  1. The model hub section in .github/dependabot.yml.
  2. Install model_hub section in .github/lint-python.yml.

@MikhailKardash MikhailKardash requested a review from a team as a code owner August 29, 2024 17:30
@MikhailKardash MikhailKardash requested a review from jgongd August 29, 2024 17:30
@determined-ci determined-ci requested a review from a team August 29, 2024 17:30
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

jgongd
jgongd approved these changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@jgongd jgongd left a comment

Choose a reason for hiding this comment

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

LGTM

@jgongd
Copy link
Contributor

jgongd commented Sep 3, 2024 •
edited
Loading

@azhou-determined! Could you take a look at this too? I know you've done a ton of work on the harness side.

azhou-determined
azhou-determined approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@azhou-determined azhou-determined left a comment

Choose a reason for hiding this comment

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

lgtm.

i approved and ran the e2e-longrunning tests. there's a few failures but they don't seem related to this PR, though i didn't dig too deeply.

@mackrorysd
Copy link
Member

Failures all disappeared on retry, and are being looked into elsewhere as flaky. The failure that remains is the one with the ChromaDB version. But these e2e test runs aren't free, so I'm not going to rebase and re-run - I think we've established that this patch passes tests. Time to merge!

@mackrorysd mackrorysd merged commit b6eb05e into main Sep 6, 2024
120 of 122 checks passed
@mackrorysd mackrorysd deleted the remove_model_hub branch September 6, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgongd jgongd jgongd approved these changes

@tara-hpe tara-hpe tara-hpe approved these changes

@azhou-determined azhou-determined azhou-determined approved these changes

@dannysauer dannysauer dannysauer approved these changes

@caehd10 caehd10 Awaiting requested review from caehd10

Assignees

@azhou-determined azhou-determined

Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@MikhailKardash @determined-ci @jgongd @mackrorysd @dannysauer @azhou-determined @tara-hpe