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
Workflow - Build Conda Package #1870
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1870 +/- ##
========================================
+ Coverage 90.2% 100.0% +9.8%
========================================
Files 293 293
Lines 23915 23915
========================================
+ Hits 21570 23905 +2335
+ Misses 2345 10 -2335
Continue to review full report at Codecov.
|
…valml into 1824-Build-Conda-Package
…valml into 1824-Build-Conda-Package
# Conflicts: # docs/source/release_notes.rst
…valml into 1824-Build-Conda-Package
…valml into 1824-Build-Conda-Package
@@ -23,7 +23,7 @@ scikit-optimize==0.8.1 | |||
scipy==1.6.2 | |||
seaborn==0.11.1 | |||
shap==0.39.0 | |||
sktime==0.5.3 | |||
sktime==0.6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this dependency change for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oo this was for the dependency test right? I just merged in the PR so this isn't needed anymore 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this was unrelated but needed to pass the dependency check, sktime had a release a few hours ago and it doesn't break any of our code but I thought I should include it for bug fixes etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^Ignore this haha I didn't see your next comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ParthivNaresh I think this looks good!
name: Build Conda Package | ||
|
||
on: | ||
pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only want this to run on pushes to main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freddyaboulton Currently build_conda_pkg
runs in CircleCI as part of the other workflows, so on every commit, primarily for the shellcheck
. Are we changing this behaviour @dsherry ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think I missed if: github.ref == 'refs/head/main'
line below. If that means that the package is only built on commits to main, then I think we're good to go!
But I think we can make it simpler by only using
on:
push:
branches:
-main
I don't think we need the shellcheck to run on PR branches but let me know if I'm misunderstanding!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I was under the impression we were keeping the same behaviour as the CircleCI implementation but if we're good to not do a shellcheck on PR branches then I can definitely make the change to only run on main
. I agree that I don't think it's necessary to have it run on PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
I also wonder if shellcheck would belong better in our lint
jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! One minor change requested, to prove that the action is green
username: $DOCKERHUB_USERNAME | ||
password: $DOCKERHUB_PASSWORD | ||
environment: | ||
OMP_NUM_THREADS: 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ParthivNaresh @freddyaboulton I see our old config was setting OMP_NUM_THREADS
here. I believe this config was being applied for all our circleci jobs. I am wondering if we should continue to set this in our GH actions.
@@ -1,101 +0,0 @@ | |||
version: 2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 🚽 🌪️ 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goooodbye codecov!! LGTM 🥂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 😁
…skEngine`` #1975. - Added optional ``engine`` argument to ``AutoMLSearch`` #1975 - Added a warning about how time series support is still in beta when a user passes in a time series problem to ``AutoMLSearch`` #2118 - Added ``NaturalLanguageNaNDataCheck`` data check #2122 - Added ValueError to ``partial_dependence`` to prevent users from computing partial dependence on columns with all NaNs #2120 - Added standard deviation of cv scores to rankings table #2154 - Fixed ``BalancedClassificationDataCVSplit``, ``BalancedClassificationDataTVSplit``, and ``BalancedClassificationSampler`` to use ``minority:majority`` ratio instead of ``majority:minority`` #2077 - Fixed bug where two-way partial dependence plots with categorical variables were not working correctly #2117 - Fixed bug where ``hyperparameters`` were not displaying properly for pipelines with a list ``component_graph`` and duplicate components #2133 - Fixed bug where ``pipeline_parameters`` argument in ``AutoMLSearch`` was not applied to pipelines passed in as ``allowed_pipelines`` #2133 - Fixed bug where ``AutoMLSearch`` was not applying custom hyperparameters to pipelines with a list ``component_graph`` and duplicate components #2133 - Removed ``hyperparameter_ranges`` from Undersampler and renamed ``balanced_ratio`` to ``sampling_ratio`` for samplers #2113 - Renamed ``TARGET_BINARY_NOT_TWO_EXAMPLES_PER_CLASS`` data check message code to ``TARGET_MULTICLASS_NOT_TWO_EXAMPLES_PER_CLASS`` #2126 - Modified one-way partial dependence plots of categorical features to display data with a bar plot #2117 - Renamed ``score`` column for ``automl.rankings`` as ``mean_cv_score`` #2135 - Fixed ``conf.py`` file #2112 - Added a sentence to the automl user guide stating that our support for time series problems is still in beta. #2118 - Fixed documentation demos #2139 - Update test badge in README to use GitHub Actions #2150 - Fixed ``test_describe_pipeline`` for ``pandas`` ``v1.2.4`` #2129 - Added a GitHub Action for building the conda package #1870 #2148 .. warning:: - Renamed ``balanced_ratio`` to ``sampling_ratio`` for the ``BalancedClassificationDataCVSplit``, ``BalancedClassificationDataTVSplit``, ``BalancedClassficationSampler``, and Undersampler #2113 - Deleted the "errors" key from automl results #1975 - Deleted the ``raise_and_save_error_callback`` and the ``log_and_save_error_callback`` #1975 - Fixed ``BalancedClassificationDataCVSplit``, ``BalancedClassificationDataTVSplit``, and ``BalancedClassificationSampler`` to use minority:majority ratio instead of majority:minority #2077
Fixes #1824