CI: Linting with azure instead of travis
State: closed

closes #22844

Moving all the linting, other code checks and doctests to azure. 

Labels:
name: CI
description: Continuous Integration
name: Code Style
description: Code style, linting, code_checks

Comments:
Looks like this requires a bit more research. I'm not activating the conda environment, so I was guessing whether this would fail or not. And it seems it does.

This answers your question. Defining them as steps, when one fails, the job is interrupted. An alternative would be to create a job for each, but I think that will create too many jobs, and it'll make it difficult to find the jobs of the tests. Will do some research and see if it's possible to continue the steps when one is failing.

I think yaml should support having scripts with multiple lines, I'll give that a try. I think that should be neater than having a script for each check with a loop.
https://docs.microsoft.com/en-us/azure/devops/pipelines/yaml-schema?view=vsts#job

Looks like there's a `continueOnError` field that determines this. That can be set at the job or script level. We'll want `true`.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22854?src=pr&el=h1) Report
> Merging [#22854](https://codecov.io/gh/pandas-dev/pandas/pull/22854?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/45f880b63e7fbbed832c4f4d7eec0c617520d1c8?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22854/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22854?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22854      +/-   ##
==========================================
- Coverage   42.45%   42.44%   -0.01%     
==========================================
  Files         161      161              
  Lines       51561    51559       -2     
==========================================
- Hits        21888    21886       -2     
  Misses      29673    29673
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `42.44% <100%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22854?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `42.02% <ø> (ø)` | :arrow_up: |
| [pandas/core/tools/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvdGltZWRlbHRhcy5weQ==) | `66.66% <ø> (ø)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `38.7% <ø> (ø)` | :arrow_up: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `16.66% <ø> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <ø> (ø)` | :arrow_up: |
| [pandas/core/window.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvd2luZG93LnB5) | `28.93% <100%> (ø)` | :arrow_up: |
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `36.97% <0%> (-0.15%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `63.41% <0%> (-0.08%)` | :arrow_down: |
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/22854/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `33% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22854?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22854?src=pr&el=footer). Last update [45f880b...498cebb](https://codecov.io/gh/pandas-dev/pandas/pull/22854?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Hello @datapythonista! Thanks for updating the PR.

 - There are no PEP8 issues in the file [`scripts/generate_pip_deps_from_conda.py`](https://github.com/datapythonista/pandas/blob/eb558dfd6575d038553a77acec79f73c1a4f7c25/scripts/generate_pip_deps_from_conda.py) !



##### Comment last updated on November 12, 2018 at 16:58 Hours UTC
@TomAugspurger seems like the `continueOnError` is not working at the job level (it should based on the docs), it needs to be at step level. But more important, continuing means that the build will be successful when an step fails, so it's not an option.

I think the log is much clearer, I personally thing it'd be great if we could use azure for the linting. But I think the drawbacks are probably deal breakers, and I don't think they can be fixed:
- We can't run all the linting/checks, seems like when one in each job fails it has to stop. And I don't think we want to have one job per test, that would make things much more complicated (both in the dashboard and in the code).
- Having the checking in the yaml, we can't run `./ci/lint.sh` locally. We could have a script that parses the yaml and executes all the checks, or we could build the yaml from a script. But I don't think any of them is actually worth.

Thoughts?

CC @chrisrpatterson
Looks like `continueOnError` is converting the error in a warning, but adding `condition: true` is what makes that a step is executed no matter the outcome of the previous.

This looks cool now:
https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=367&view=logs

And seems like we can have links to the source code line with the linting error, which may also be helpful in some cases.

I guess we still want to be able to run all the linting and checks locally. I think a script that parses the yaml and execute every command on it should be really easy, and would do the work for now.

Thoughts? @TomAugspurger @jreback 
Yes, that looks quite nice. Thanks.

As an alternative to parsing the YAML, could we have a directory of scripts
like `ci/lint/lint-py.sh`, `ci/lint/lint-c.sh`. Each of those would be one
of the `script` lines in your YAML.

Then we could have a `ci/lint.sh` that loops over all the `ci/lint/*.sh` to
run them all?

On Sat, Sep 29, 2018 at 5:13 AM Marc Garcia <notifications@github.com>
wrote:

> Looks like continueOnError is converting the error in a warning, but
> adding condition: true is what makes that a step is executed no matter
> the outcome of the previous.
>
> This looks cool now:
>
> https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=367&view=logs
>
> And seems like we can have links to the source code line with the linting
> error, which may also be helpful in some cases.
>
> I guess we still want to be able to run all the linting and checks
> locally. I think a script that parses the yaml and execute every command on
> it should be really easy, and would do the work for now.
>
> Thoughts? @TomAugspurger <https://github.com/TomAugspurger> @jreback
> <https://github.com/jreback>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22854#issuecomment-425633664>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIqDamrYYcQ5DYWaC0mzikWqSj0Ziks5uf0exgaJpZM4W8qbh>
> .
>

I unified all the linting of `*.py` files in #22863 (https://github.com/pandas-dev/pandas/pull/22863/commits/adcda478c3af1ae0edeaf09630ce2c3d6926b36a), which I think that simplifies things, and adds few value to have them separate, even in azure.

But even with that we'd have more than 10 scripts. And while that would surely guarantee the syncronization between azure and `lint.sh` in the exact commands, I think we can end up having differences on which scripts are called in each place. And, I think we'd loose the command being executed (e.g. we'd see in the logs `./ci/check_deprecated_directive.sh` instead of `grep -v ".. deprecated::`, which I think makes things more difficult to debug).

So, I think I prefer another option (which look much simpler to me):
- We could simply not have anything to call all checks (linting .py files is as simple as `flake8 .` with the current state of #22863, and the other checks we can probably wait for the CI)
- Have the `lint.sh` that iterates over `azure-pipelines.yml` and run its commands
> But even with that we'd have more than 10 scripts.

My only real thought here is that I prefer a centralized ci/lint.sh to less-centralized.  I have a hard enough time keeping track of what is checked where as it is.
Hey @datapythonista -- you asked me in another PR to take a look here. I'm working on getting #23111 back in good shape. What were you specifically looking for in this one?
Thanks @vtbassmatt.

The idea here is to move the code validations from travis to azure-pipepines. Mainly because the presentation is much better, and it's much easier to find the code errors.

A general look would be useful, to see if it makes sense what I did. But there are few more things that would be great to see if can be done and how.

- Adding the same format used in flake8 to cpplint, the doctests...
- See if there is away to avoid creating a miniconda environment and compiling the code just for the linting (e.g. reusing the environment from another job)
- Pushing the errors to a github comment (same as pep8speak does, which would not be necessary anymore)
- Creating links from the error messages to the location of the failing code. A colleague of yours said we would get them if using the format we are using, but I don't see links being created.

And anything else you think could make better use of azure-pipelines for our code checks.
Gotcha, thanks. I'm not super deep on doctests (and never used cpplint before!) so I don't know if I can accelerate you there.

Avoiding the recompile: each job runs on a new agent, and no files are retained between them. So there are two options:
1. Slot in the linting after one of the existing build jobs. This can be done as a template so it's still logically separate, but this way it would run on the same agent.
2. Publish build artifacts at the end of one job, then consume them at the start of another.

Let me know which of those seems more appealing and I can put something together. Personally I prefer the former.

pep8speaks is cool; I had never seen that before! I need to do some thinking about the model here, but I think at a minimum, we could publish an example of how to do that. @DavidStaheli you might be interested in this as well.

Links to files: from casual inspection, what you're doing looks right. @azureDaveOps do we have a bug in how we show links to error lines in code files?
Not sure about the details, but having a job that publish an artifact sounds actually really great. Not only for the linting, but for everything. If we can have a job that downloads the source code of the PR and the miniconda installer, and we don't need to repeat that in every job, we would save a decent amount of resources.

It would probably be even better if we could reuse the artifact with the conda dependencies already installed from one build to the next, but I don't know if that's easy, and we should answer questions in our side like when to expire that.

Having the linting in one of the testing jobs is what we have now in Travis. But I really like how things are presented know in separate jobs in the azure dashboard. Having the lint failures in a job "Testing linux" is not as clear as having it in "Code checks" in my opinion.

Thanks a lot for the help with this.
@datapythonista 

> See if there is away to avoid creating a miniconda environment and compiling the code just for the linting (e.g. reusing the environment from another job)

we won't ever do this. From long experience this often ends up being pretty complicated, and is just asking for trouble. you want completely independent builds, that have NO interactions.
Fair enough. I guess for the linting, as it's already with one of the builds, we can see if it's not complicated to do the same, but in a separate job. I think this will be same as now, but I think it'll make things clearer if it appears in a separate job in the dashboard (as it's shown in the CI of this PR already).
I'm hoping to find some time to crank this out for you, but it won't be until tomorrow at the earliest. In the meantime, here's a rough (untested) sketch of the setup you'll want:

```yaml
jobs:
- job: Build
  steps:
  - script: echo Do my build stuff!
  - task: CopyFiles@2
    inputs:
      #sourceFolder: # ideally, your build outputs would be in some particular subdir, that way you don't have to publish your entire source and intermediate build results
      targetFolder: $(Build.ArtifactStagingDirectory)
  - task: PublishBuildArtifacts@1

- job: Lint
  dependsOn: Build  # this is the name we gave the job that publishes build artifacts
  steps:
  - task: DownloadBuildArtifacts@0  # this will download all the build artifacts from previous jobs
  - script: echo Do my linting stuff!
    workingDirectory: $(System.ArtifactsDirectory)
```

The last 2 things the Build job does are copy files to a staging directory and then upload them as a build artifact. The copy step isn't _strictly_ necessary, but it makes it easier to grab various pieces from the build tree and stage them for a single upload. By default, the artifact is called `drop` but you can change that with [arguments to the Publish task](https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/publish-build-artifacts?view=vsts).

In that Lint job, we'll still download source first. Then the first task is to grab the build artifacts from previous steps. Artifacts will be downloaded into $(System.ArtifactsDirectory) by default. This is changeable with some [inputs to the task](https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/download-build-artifacts?view=vsts). What I'm not 100% on is behavior like, what exact directories do we lay things down into, what if there are filename conflicts, etc.
(And soon I hope to wrap some of that up with native YAML syntax, because having to know about 3 different tasks is kinda hostile.)
Adding screenshot of how the errors display, before I fix them, so the PR can be merged:

![azure_errors](https://user-images.githubusercontent.com/10058240/47961709-1ca90800-e008-11e8-80d7-cccc2c2e5776.png)

and the link:
https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=2349&view=logs
@TomAugspurger @jreback I'd merge this PR as it is. There is some more work to do, like trying to write github comments with the azure results, and reusing the conda of another job for the linting, but I'd take care of them in separate PRs.

Let me know if this looks good, or this needs any change (note that the CI for this will fail until #23486 is merged).
is this actually building in the code checks job?
all of the long checks don’t need a build but the doctest do
That's a very good point. No, I just realize now, that the linting works, but the parts that depend on the build don't. Thanks!
so it might be ok to move just the doctests back to the travis build 
That's surely an option. I can also move the linting to the linux test job, so that would be equivalent to what we have now in travis.

And I also can try what @vtbassmatt suggested, and publish the files of one of the builds (built source, conda environment), and reuse them for the code checks job. This is a bit more complex, but the results would be the clearest in the azure dashboard, so it's probably worth giving it a try.

I'll do some research and I'll be back with what I think it's the best approach.

Thanks!
Related to this, I created an issue in `pytest` to see if we can show the pytest errors in azure in the same way that we do with `flake8`: https://github.com/pytest-dev/pytest/issues/4311 
@TomAugspurger @jorisvandenbossche can you take a look at what I'm doing with the docs here, to see if you think it's reasonable, before I continue with this path?
Just to make sure I understand: the linting and doc build will be done on the same job right? That all seems reasonable to me.
Yes, that's what I'm doing here. I'm not sure, but I think travis has a timeout that azure doesn't, so that's an advantage of moving it here.

And I found it simpler, as the job has a script of its own, we don't need the `LINT=true` or `DOC=true` that we used in travis. And for me at least, it's conceptually more clear to find all the non-test failures in a single job.

I'm open to other structures, I was just testing options, but personally I'm quite happy how this looks like. My idea is to also add the `validate_docstrings.py` here (after #23560 is merged).
Sounds good. IIRC the main reason the doc build as moved to allowed
failures was because it was timing out.

On Sat, Nov 10, 2018 at 4:28 PM Marc Garcia <notifications@github.com>
wrote:

> Yes, that's what I'm doing here. I'm not sure, but I think travis has a
> timeout that azure doesn't, so that's an advantage of moving it here.
>
> And I found it simple, as the job has a script of its own, we don't need
> the LINT=true or DOC=true that we used in travis. And for me at least,
> it's conceptually more clear to find all the non-test failures in a single
> job.
>
> I'm open to other structures, I was just testing options, but personally
> I'm quite happy how this looks like. My idea is to also add the
> validate_docstrings.py here (after #23560
> <https://github.com/pandas-dev/pandas/pull/23560> is merged).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22854#issuecomment-437626522>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIsDECi2Wq3EU1at7v3z6WIvL-hpZks5ut1L2gaJpZM4W8qbh>
> .
>

is there a reason to conflate the docs & code checks on a single build?

why not just create another azure build for code-checks? e.g. you could then run the linting check very early and fail the build if that doesn't pass

then buidl & run the rest of the code checks.

I know it seems heavy weight to create another build, but azure is pretty fast.
Do you mean that if the build with only the code checks fail, we should cancel building the docs and the builds of the tests?

The main reason was as you guessed to have one less build, so we save setting up the environment and building pandas once.