Conda Environment task #7075

brcrista · 2018-04-25T19:40:31Z

Testing

L0
Manually on self-hosted Windows and Linux, using both pre-existing Conda installations and installing the lastest Conda
Manually on Hosted VS2017 and Hosted macOS

hashtagchris · 2018-04-25T23:17:26Z

For persistent self hosted agents, if our task installs Conda, will $CONDA be set the next time the task runs and prevent install/update to newer versions?

davidstaheli · 2018-04-25T23:22:30Z

Regarding @hashtagchris' comment, when Miniconda gets installed, could it be installed in the tool cache so that it sticks around between builds on self-hosted agents? The task could look in this order to find 'conda' at runtime: $CONDA, then the tool cache, then download it.

hashtagchris · 2018-04-25T23:29:57Z

BTW, I meant to suggest that preventing the update of newer versions is generally a bad thing. :)

My thinking: If conda was installed by a previous task execution and installConda is still checked, let's call conda update --name base conda --yes unless it takes a long time. If conda was installed independently, let's leave it alone.

brcrista · 2018-04-26T01:14:59Z

@hashtagchris @davidstaheli What David said from the folks he talked to at Anaconda Con, there doesn't seem to be much value in providing access to older / side-by-side versions of Conda.

Version slippage on private agents is a problem. I like the idea of the task handling update. I say we change "Install Conda if missing" to "Get latest Conda." It will install if it's missing, or if it finds an existing one, will go ahead and update it.

Regarding $CONDA, I don't know of any way for the task to set a system variable on the host machine like that. I don't know if that's even desirable. So right now, all it does is set $CONDA for the rest of the build process. We could just hardcode the task to go look where it installs Conda if $CONDA isn't set. Open to suggestions here.

lkillgore · 2018-04-26T12:22:42Z

Tasks/CondaEnvironment/conda.ts

+
+interface TaskParameters {
+    environmentName: string,
+    packageSpecs?: string


nit: there's a missing comma here. I'm kinda surprised typescript didn't catch this. I guess this is just a lint error? Are you running a lint check?

Weird! I thought that would be a syntax error. I should probably take the time to set up a linter.

lkillgore · 2018-04-26T12:30:33Z

Tasks/CondaEnvironment/conda.ts

+    installConda?: boolean
+}
+
+export async function condaEnvironment(parameters: Readonly<TaskParameters>, platform: Platform): Promise<void> {


nit: I feel like this could have a better name. condaEnvironment is a function without a verb in the name: useCondaEnvironment or the like would be an improvement, IMO.

I named the function for the task. The pattern here is that main.ts communicates with the task runner, and this function contains the actual implementation for the task.

To the point, perhaps the name of the task could have a verb in it. We could say "Activate Conda Environment," but that makes it sound like the environment has to exist already.

I feel that code should read like code. To me, that's more important than this matching a design pattern. Conda Environment may make sense as a name for a task, but it doesn't make sense as a function. If you feel that this pattern is important enough to change the name of the task, I'd support that decision; I don't think that the name of the task should dictate the name of the function in the code if makes the code read weird.

I see your point, but here's why I don't think it sounds so weird:

There's a paradigm where "variables are nouns and functions are verbs." But would you rather say father('Lucas Killgore') or const father = getFather('Lucas Killgore')? What about father(mother(father('Lucas Killgore')))? Since your father never changes, the function might as well memoize the result, so you only do the "getting" once -- is it still getFather, or just father now?

My point is, if functions are your main unit of code organization, not all of your functions are verbs. And so the condaEnvironment function is our abstract representation of the task -- just as you would use a class as the abstraction of the task in C#. But JavaScript has made the design decision to make functions easy and classes clunky, thus we use a function.

I'm not sure I follow your example. I would say getFather. Caching would be an implementation detail. father is a verb, and I would expect it would get the child of Lucas Killgore, which in this case would be null, or empty, or possibly even stuffed panda bears depending on definitions. From your comment, I would expect that you would think it should return Darth Vader. In any case, leaving 'get' off makes the call ambiguous.

FWIW, this would be fine to me: const environment = new CondaEnvironment(); environment.set();.

I think it comes down to me disagreeing with the central conceit: 'if functions are your main unit of code organization, not all of your functions are verbs'. No, I think they should be, or at least, this is not an exception to that case.

lkillgore · 2018-04-26T12:36:05Z

Tasks/CondaEnvironment/conda_internal.ts

+
+    // By default `downloadTool` will name the downloaded file with a GUID
+    // But on Windows, the file must end with `.exe` to make it executable
+    const tempDirectory = task.getVariable('AGENT_TEMPDIRECTORY');


I think it's actually AGENT.TEMPDIRECTORY (at least that seems to be what most other tasks use... I think any . get converted to _ under the covers. Only the tests seem to use _.)

Both will work

I'm not sure both are 'correct' the only other tasks that use _ are those that by-pass the framework and directly read the environment variable.

The dot notation seems more VSTS native to me. For the dot notation, Agent.TempDirectory is the canonical casing. Interesting that this variable isn't documented at https://docs.microsoft.com/en-us/vsts/build-release/concepts/definitions/build/variables

Fair enough. I'll change it to use the dot.

For what it's worth, the Agent.ToolsDirectory variable isn't documented either.

lkillgore

Looks good to me.

davidstaheli · 2018-04-26T14:14:48Z

Side question:
@TingluoHuang and @ericsciple, should Agent.TempDirectory be documented with other variables at https://docs.microsoft.com/en-us/vsts/build-release/concepts/definitions/build/variables?

hashtagchris · 2018-04-26T14:35:29Z

I thought it might break a user's expectations if we updated their system's installation of Conda, but FWIW I see that Apple App Store Release updates your system's fastlane using gem update fastlane

…not the same one that had the method invoked on it

…system's Conda installation. Enable by default

brcrista · 2018-04-26T17:55:21Z

After discussing with @madhurig, we decided just to rip out the installer part. It works fine, but our other tasks don't work this way, and it pulls down a 1.5+ GB download. We'll still have the option to keep an existing installation (hosted or private) up-to-date.

Documentation to go with microsoft/azure-pipelines-tasks#7075 Related work items: #1239276

brcrista added 30 commits April 20, 2018 13:13

Scaffold out CondaEnvironment task

885f1bd

Update icon

1b14441

Stub out task implementation

158d602

install Sinon for tests

28baa72

Add integration tests

0446d0a

Overload reload

0653163

Use switch to make the pattern more extensible

76c203c

Add failing tests for findConda and downloadConda

0b8bada

Add doc comments. Add installer function

99a9cb3

Add mocks for installMiniconda

69ac784

Rearrange tests

f13ee80

Refactor the CONDA env var to live entirely in conda.ts

5137b8a

All mocks of functions that return absolute paths return absolute paths

dedcb6b

test gap for hasConda

f522c1e

Fix tests broken because of bad environment path

eb0980e

else if

3a371cc

Test activating the Conda environment

3ae3537

test for installing Miniconda

6d7a76b

Set CONDA build var after installing Miniconda

fc9b296

Implement downloading miniconda

6d4fb56

Add Conda to PATH. Add test for creating the environment

38fdea2

Update doc comment for createEnvironment

eac613d

conda_internal tests must mock tool lib

e5d7cc6

Implement hasConda

bd0dfdb

Implement Miniconda installer

d7ec9fe

Implement create environment

c87820f

Implement activate environment

bb3ec9e

Fix create conda env

ed3c110

Separate test suites for conda and conda_internal

3837b96

Split tests into separate files

bd88fb9

lkillgore reviewed Apr 26, 2018

View reviewed changes

lkillgore approved these changes Apr 26, 2018

View reviewed changes

brcrista added 4 commits April 26, 2018 10:54

UX change: option to update an existing Conda installation

3057263

Sinon has weird chaining behavior. Apparently the object returned is …

5b522ea

…not the same one that had the method invoked on it

Use dotted variable names

7d977f6

Make it more clear that the "get latest Conda" option may update the …

830da6b

…system's Conda installation. Enable by default

brcrista mentioned this pull request Apr 26, 2018

Install Miniconda on Linux container microsoft/vsts-agent-docker#97

Merged

Feedback on UI strings. Add "clean" parameter

59cef30

brcrista added 6 commits April 26, 2018 14:30

Rip out downloader and installer

74ea55d

Fix bug with finding conda through PATH

456877e

Forgot to pass user-provided options to create

f6a1307

Change category to "Package"

1f311bb

Add ThirdPartyNotice.txt

9bb7821

Update strings from doc review

1bba15e

Taojunshen pushed a commit to MicrosoftDocs/azure-devops-docs that referenced this pull request May 4, 2018

Merged PR 337919: Document "Conda Environment" task

f1455d6

Documentation to go with microsoft/azure-pipelines-tasks#7075 Related work items: #1239276

brcrista added 5 commits May 7, 2018 09:12

add link to documentation. bump version number

49e8ca4

New npm minor version changed package-lock.json on build

e66a984

Merge branch 'master' into users/brcrista/conda-task

cc6cea3

Rename folder to CondaEnvironmentV0

b6821da

Add end-to-end tests

8f7751e

brcrista merged commit 054a710 into master May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conda Environment task #7075

Conda Environment task #7075

brcrista commented Apr 25, 2018 •

edited

Loading

hashtagchris commented Apr 25, 2018

davidstaheli commented Apr 25, 2018

hashtagchris commented Apr 25, 2018

brcrista commented Apr 26, 2018

lkillgore Apr 26, 2018

brcrista Apr 26, 2018

lkillgore Apr 26, 2018

brcrista Apr 26, 2018

lkillgore Apr 26, 2018

brcrista Apr 26, 2018

lkillgore Apr 26, 2018

lkillgore Apr 26, 2018 •

edited

Loading

brcrista Apr 26, 2018

lkillgore Apr 26, 2018

hashtagchris Apr 26, 2018

brcrista Apr 26, 2018

lkillgore left a comment

davidstaheli commented Apr 26, 2018

hashtagchris commented Apr 26, 2018

brcrista commented Apr 26, 2018

Conda Environment task #7075

Conda Environment task #7075

Conversation

brcrista commented Apr 25, 2018 • edited Loading

hashtagchris commented Apr 25, 2018

davidstaheli commented Apr 25, 2018

hashtagchris commented Apr 25, 2018

brcrista commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkillgore Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkillgore left a comment

Choose a reason for hiding this comment

davidstaheli commented Apr 26, 2018

hashtagchris commented Apr 26, 2018

brcrista commented Apr 26, 2018

brcrista commented Apr 25, 2018 •

edited

Loading

lkillgore Apr 26, 2018 •

edited

Loading