[make] rewrite modified_py_files in python to be cross-platform
Author: stas00
Pull request: yes

As it can be seen here https://github.com/huggingface/transformers/pull/8359 some windows setups don't do well with the few lines of unix code that derive which files were modified, when called from `Makefile` so it looked like the best solution was to rewrite that logic in python which this PR does.

**status update:** If I understand correctly this PR works on windows @sgugger uses, but not for @jplu, but it should be merged regardless as it removes the hurdle reported in https://github.com/huggingface/transformers/pull/8359, which was preventing @jplu from using any `make` targets. Since `fixup` is an optional target and isn't a show stopper I recommend we merge this and then move the remaining issue into a separate issue and then find a windows expert to help make this target work for all windows users.

The rest of the post is the original request to validate that the PR works on windows.

----------------------------

@jplu  - please kindly validate that this works on windows. test it with:
1. no changes to any files under src utils tests examples


```
make fixup
```
this one should not fail but not do much either, my output is:
```
No library .py files were modified
python utils/check_copies.py
python utils/check_dummies.py
python utils/check_repo.py
Checking all models are properly tested.
Checking all models are properly documented.
python utils/style_doc.py src/transformers docs/source --max_len 119
```


2. with 1 change in some .py file under 2 of the above dirs, by hand or via `echo` if it works on windows:
```
echo -n "#" >> tests/conftest.py 
echo -n "#" >> examples/README.md 
echo -n "#" >> src/transformers/testing_utils.py
make fixup
```
this time I get:
```
Checking/fixing src/transformers/testing_utils.py tests/conftest.py
reformatted /mnt/nvme1/code/huggingface/transformers-fixup-python/tests/conftest.py
reformatted /mnt/nvme1/code/huggingface/transformers-fixup-python/src/transformers/testing_utils.py
All done! ✨ 🍰 ✨
2 files reformatted
python utils/check_copies.py
python utils/check_dummies.py
python utils/check_repo.py
Checking all models are properly tested.
Checking all models are properly documented.
python utils/style_doc.py src/transformers docs/source --max_len 119
```

If all is good you should get the same output as mine, sans different base path.

The key is to see:
```
Checking/fixing src/transformers/testing_utils.py tests/conftest.py
```
it may include `utils/get_modified_files.py` if you're checking out this PR as it's under git too, it doesn't matter. What we want to ensure is that out of 3 intentionally modified files 2 gets looked at as they are ending with `.py`.

Just in case you don't know how to checkout a pr, you can for example use `gh` https://github.com/cli/cli:
```
gh pr checkout 8371
```
but there are many other ways to do it.

Thank you!

@LysandreJik, @sgugger 

Comments:
Nice!! Now I have with `make fixup`:

```
-n was unexpected.
make: *** [modified_only_fixup] Error 255
```

OK, so it sounds that perhaps I was trying to fix the wrong thing in first place. Let's see if we can find a different way to check whether the variable is empty that also works on windows.

@jplu, please try now - I used `test -n` instead.

same error

ok, can you perhaps tell me which test would work on windows? 

We are doing a very simple thing
```
if len(foo):
  do a
else:
  do b
```
what's a good test for us to use for `len(foo)` on windows?

also what's your `test -h` shows - perhaps no `-n` option there?

on unix:
```

       -n STRING
              the length of STRING is nonzero

       STRING equivalent to -n STRING
```

does it work if you just remove "-n", i.e. `test -n` and just `test` are supposed to do the same.


Doesn't matter which command I do with `test` it doesn't works. I don't think we will succeed to run any shell code, only the commands are available not the shell interpreter. Would it be possible to turn that into a Python subprocess that runs itself the `black`, `isort` and `flake8` command?

It's possible, but surely there must be a way to do such a basic thing on windows.

How about this?

```
	@if [ "$(modified_py_files)" != "" ]; then \
		echo "Checking/fixing $(modified_py_files)"; \
	else \
		echo "No library .py files were modified"; \
	fi
```

If that doesn't work either, perhaps it doesn't like the spacing? remove space before `\`?

I get:
```
"utils/check_repo.py" was unexpected.
make: *** [modified_only_fixup] Erreur 255
```

With:

```
$(eval modified_py_files := $(shell python utils/get_modified_files.py $(check_dirs)))
@if [ "$(modified_py_files)" != "" ]; then \
	echo "Checking/fixing $(modified_py_files)"; \
else \
	echo "No library .py files were modified"; \
fi
```

"utils/check_repo.py" is the value of `$(modified_py_files)`

Thank you for checking that and overall your patience so far.  

This back-n-force doesn't seem to lead to a productive way of sorting it out. I need to have direct access to experiment on.

Is there a way I could get access to a windows box with a similar setup to yours - I won't suppose there is google colab like environment but on windows?

@sgugger, is there someone on the hf team who groks windows shell/make and could help us out here? We need to be able to do such basic things in `Makefile` as checking if a variable is not "".

If it can help, PowerShell is an opensource project https://github.com/PowerShell/PowerShell so you can install it (on Mac or Linux) to have a Windows interpreter like mine. But I don't know if you will have access to your tools from it, never tried this can of thing on my Linux.


I don't think there is anyone that can help, maybe @mfuntowicz ? I personally only use WSL on Windows on my laptop, so the make commands work without any problem

OK, so since the problematic on windows code is now hidden in the optional target, we can go ahead and merge this PR - as it no longer interferes with the normal functioning of Makefile for @jplu and other users on the same setup. And meanwhile we will look for a solution for this basic check. How does that sound, @jplu? 

One possible solution would be to require windows users to use one of the approved set of shells, since as @sgugger says he doesn't have this issue on his windows box. Does it make sense?

> If it can help, PowerShell is an opensource project https://github.com/PowerShell/PowerShell so you can install it (on Mac or Linux) to have a Windows interpreter like mine. But I don't know if you will have access to your tools from it, never tried this can of thing on my Linux.

Thank you, @jplu. I will give it a quick try. And if it takes too long I will defer to someone who is a windows user to sort this out, but it doesn't need to keep us from merging this fix.

@jplu, fwiw I installed PowerShell and everything works there just fine:
```
/hf/transformers-fixup-python> pwsh
PS /hf/transformers-fixup-python> make fixup
Checking/fixing src/transformers/testing_utils.py utils/get_modified_files.py
All done! ✨ 🍰 ✨
2 files left unchanged.
python utils/check_copies.py
python utils/check_dummies.py
python utils/check_repo.py
2020-11-06 21:02:02.519250: I tensorflow/stream_executor/platform/default/dso_loader.cc:49] Successfully opened dynamic library libcudart.so.11.0
Checking all models are properly tested.
Checking all models are properly documented.
python utils/style_doc.py src/transformers docs/source --max_len 119
```

Either of these works:
```
       @if test -n "$(modified_py_files)"; then \
       @if [ "$(modified_py_files)" != "" ]; then \
```


I confirm it is still not working on my side, there should be some differences between both version. I think in your case it is biased because you have access to a shell code interpreter. I checked a bit how to do a `if` in usual PowerShell and apparently it is not the same way https://docs.microsoft.com/en-gb/powershell/scripting/learn/deep-dives/everything-about-if?view=powershell-7 they don't if the `if` and `fi`. So on your side I think that `make` is still using your usual shell and not powershell.



So we need a not shell-specific way.

I don't know anything about the Windows world - but is the shell you use better/preferable to the one that @sgugger uses, which he says doesn't have this problem?

Unrelated, could you please confirm that with this PR the rest of the `Makefile` works for you - that is you have no trouble running all targets, except `fixup`?

As I mentioned `fixup` is an optional target, as long as `style` and `quality` targets work. The former is much faster, but it's not required for the normal operation. And I trust that eventually we will find a way to sort it out for all setups.

I'm using as well the one that uses @sgugger which is not a simple shell, but a real Linux distribution installed on Windows (mine for example is Ubuntu 20.04), this is called WSL, and this is mostly what I'm using. The advantage of this is that you can test your code on Windows + Linux in same time. But I don't think we can oblige the Windows users to install this.

I can confirm that the "problematic" part is only the `if` now.

This issue is not blocking as you said, I'm not against dealing with it right now that the previous issue (the long egrep command) is solved with the new Python script.

Thank you for confirming that this PR can be merged, @jplu and following up with the details on where it works.

Would you kindly open a new issue and describe the specifics of this hurdle we couldn't overcome here, ideally listing all the ways we have tried and failed and most importantly how one could reproduce this situation by having the exact setup you discovered this problem on.

Then we will seek out someone who knows how to solve this. 

I suppose this could be as simple as posting a tiny Makefile that's unrelated to `transformers` to SO, which demonstrates the issue and surely someone will have the answer.

Thank you!


I can take care of it. I will open a PR to try to solve this in a pure Python way. It should not be too difficult.

While you can do that - this is not the best way if we want to continue using `Makefile`. Otherwise just as well we do away with `Makefile` completely and write everything in python.

If we use `Makefile` we need to be able to use it properly, and because we have stumbled upon a tiny problem on some variation of Windows setup and don't have an immediate expertise to solve it is not a strong enough reason to warrant such change.

Then what about using something like?
```
OS = `uname -a`
ifeq ($(OS), Win32)
    doing some Windows here
else
    doing some Linux here
```

Absolutely! I suggested that very early on - please make a version that works for your setup and then we can integrate it with the main version. I thought you didn't know how to make it work, hence I was seeking out variations that may work for you. Your suggestion makes total sense.

Perhaps let's do it in a separate issue/PR so that we can move this one along in case some windows user is stuck needing this - but if you guys feel that there is no urgency then I will slap the [WIP] back on this PR and then we will go at it until it's perfect. Your call. I just don't want to waste time of reviewers who are probably very confused by now what to do here ;)

Let's merge this one, and I will rebase on mine to take care of this 👍 

Looks to be a know bug in the make version of Windows http://gnu-make.2324884.n4.nabble.com/process-begin-CreateProcess-NULL-quot-quot-failed-td13819.html#a13820

We have to recompile Make to make it works, so I think we won't be able to fix this issue with a pure Makefile solution.

Ok, I did install the last version of Make (4.3) and now it works perfectly. Problem solved 👍 

Amazing! Thank you for investigating this and getting to the root of it, @jplu! 

I don't know whether it's worth documenting in the CONTRIBUTING.md as a possible caveat and giving the solution?

Good idea! I will do a PR to add a section for Windows.

Added here https://github.com/huggingface/transformers/pull/8436