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

test: do not import model_hub test requirements #8771

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

kkunapuli
Copy link
Contributor

Description

Model_hub tests require building tokenizers, that are pinned to an older version and require a now old version of the rust compiler. Remove the requirement for local development; the CI imports the requirement independently.

Test Plan

Confirm all CI tests complete successfully.

Commentary (optional)

Context: https://hpe-aiatscale.slack.com/archives/CSG3W08JY/p1706227736615449?thread_ts=1691690041.149519&cid=CSG3W08JY

Checklist

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

@cla-bot cla-bot bot added the cla-signed label Jan 29, 2024
Copy link

netlify bot commented Jan 29, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 9c83cc0
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65b8309afc9f080007376c8d

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c73712b) 47.42% compared to head (9c83cc0) 44.26%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8771      +/-   ##
==========================================
- Coverage   47.42%   44.26%   -3.17%     
==========================================
  Files        1047     1019      -28     
  Lines      166976   165251    -1725     
  Branches     2242     2242              
==========================================
- Hits        79188    73144    -6044     
- Misses      87629    91948    +4319     
  Partials      159      159              
Flag Coverage Δ
backend 41.84% <ø> (-0.01%) ⬇️
harness 51.00% <ø> (-13.38%) ⬇️
web 42.50% <ø> (ø)

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

see 156 files with indirect coverage changes

@kkunapuli kkunapuli changed the title Do not import model_hub test requirements Test: do not import model_hub test requirements Jan 29, 2024
@kkunapuli kkunapuli changed the title Test: do not import model_hub test requirements test: do not import model_hub test requirements Jan 29, 2024
@kkunapuli kkunapuli requested a review from a team as a code owner January 29, 2024 22:17
@kkunapuli kkunapuli marked this pull request as draft January 29, 2024 22:20
cat <<parameters.extra-requirements-file>> >> /tmp/cachefile
for i in <<parameters.extra-requirements-file>>; do
cat $i >> /tmp/cachefile
done
Copy link
Contributor Author

@kkunapuli kkunapuli Jan 29, 2024

Choose a reason for hiding this comment

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

allow multiple requirements files to be specified; we want multiple requirements.txt files for model_hub CI test purposes, but not in the build

@kkunapuli kkunapuli marked this pull request as ready for review January 29, 2024 23:27
@kkunapuli kkunapuli merged commit 98c61f3 into main Jan 30, 2024
74 of 85 checks passed
@kkunapuli kkunapuli deleted the kunapuli/remove-modelhub-test-requirement branch January 30, 2024 17:12
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
* Do not import model_hub test requirements

* test: do not import model_hub test requirements

* include model_hub as additional requirements file

* try allowing multiple additional requirements files

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

Successfully merging this pull request may close these issues.

3 participants