-
Notifications
You must be signed in to change notification settings - Fork 834
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
feat: support direct pip install #1223
feat: support direct pip install #1223
Conversation
d205de8
to
c706c2d
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #1223 +/- ##
=======================================
Coverage 83.69% 83.69%
=======================================
Files 264 264
Lines 12911 12911
Branches 637 637
=======================================
Hits 10806 10806
Misses 2105 2105
Continue to review full report at Codecov.
|
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.
Youre just blasting through these tasks, true MVP status. left a few minor comments
- bash: echo "##vso[task.prependpath]$CONDA/bin" | ||
condition: startsWith(variables['tag'], 'v') | ||
displayName: Add conda to PATH | ||
- bash: conda info |
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.
Do we need this conda info part?
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.
Nope, it just displays the conda information, like all other jobs so I kept it.
project/Secrets.scala
Outdated
@@ -59,5 +59,6 @@ object Secrets { | |||
sys.env.getOrElse("PGP-PRIVATE", getSecret("pgp-private")).getBytes("UTF-8"))) | |||
lazy val pgpPassword: String = sys.env.getOrElse("PGP-PW", getSecret("pgp-pw")) | |||
lazy val storageKey: String = sys.env.getOrElse("STORAGE_KEY", getSecret("storage-key")) | |||
lazy val pypiAPIToken: String = sys.env.getOrElse("PYPI-API-TOKEN", getSecret("pypi-api-token")) |
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.
might be good to use this var in the build sbt step so no env vars are needed if properly authenticated
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.
Sure
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Content looks great, just a nit about replicated code
build.sbt
Outdated
writeSetupFileToTarget(dir) | ||
runCmd( | ||
activateCondaEnv.value ++ | ||
Seq(s"python", "setup.py", "bdist_wheel", "--universal", "-d", packageDir), |
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.
A lot of the code added is replicated in the project/codegen. Perhaps we can abstract this into functions and call twice?
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.
Done.
|
||
val publishPypi = TaskKey[Unit]("publishPypi", "publish synapseml python wheel to pypi") | ||
publishPypi := { | ||
packageSynapseML.value |
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.
some duplicated version getting code here.
SynapseML/project/CodegenPlugin.scala
Line 38 in bbd8744
val pythonizedVersion = settingKey[String]("Pythonized version") |
Might want to put the implementation code in an object and use it in all these diff places
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.
Done.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
No description provided.