adc-connect / adcc

adcc: Seamlessly connect your program to ADC
https://adc-connect.org
GNU General Public License v3.0
32 stars 19 forks source link

Make adcc fully open-source #106

Closed mfherbst closed 3 years ago

mfherbst commented 3 years ago

PR to track the progress of making adcc fully open-source. Closes #105.

Progress

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 17efba1d00405b1e057d229efb50a373627ab8db into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 9ed96dc7b2f8ebd68c371150c5b255644cdadb68 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 9a88850488a94d0edf58c976462dce34d51c25ff into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 733d32442a476167d42cb22d7389c4a9bf31f561 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

maxscheurer commented 3 years ago

In export_Tensor.cc here can you do

for (auto tpl : permutations) {
    if (py::isinstance<py::int_>(tpl)) {
      iterator_of_ints = true;
      break;
    }
  }

Otherwise my clang complains about the loop only running once.

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 83b77591e9e5496d289083723db9f8c5f27ce64d into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging a2b8f4b69b6c9512ef376e8aeeb1ab3e96a0c132 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging efdb847ef72b193f7aa8c411b5acbda351f4d277 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

maxscheurer commented 3 years ago

Can I make a commit here or do you have WIP that could clash?

mfherbst commented 3 years ago

I won't work on it tonight and the next things I have are to get the C++ tests working. So that's mostly stuff in libadcc and setup.py.

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 76b6d024aefbf9df74e2d19ac2a6872713848787 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging a1daf2f21527ab69efecdad40ba5fd8498ab5490 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

maxscheurer commented 3 years ago

We also should disable automatic download of libtensorlight in conda builds, such that we don't accidentally link against our "self-provided" version instead of the conda one.

mfherbst commented 3 years ago

You can. Just provide a custom siteconfig and set libtensor_autoinstall=None.

maxscheurer commented 3 years ago

But then I have to create an extra file just for the conda deployment?

Can also add echo "libtensor_autoinstall=None" > siteconfig.py in the workflow?

mfherbst commented 3 years ago

Sure. But I would write it to the home, that's more reliable (you don't depend on the working dir). It searches for ~/.adcc/siteconfig.py or so ... check the setup.py.

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 5efd7cfefa2fe2c33459021ca17b3c89ef804dfd into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 803bcc695c14cf28c6370085a7b3b85c98ed18f1 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

maxscheurer commented 3 years ago

If you make commits here, can you leave the conda builds turned on for the time being? I need a fresh adcc on dev (because of the breaking lt allocator changes).

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging 5b7c913a2797338c17554eb191796ea27e8413c4 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

maxscheurer commented 3 years ago

Nice twist, conda, not wanting the channel name that is accidentally identical with a local directory 😂😂😂

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts when merging a5496c3cc6b5279d63c65c88666a2f0170657a02 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 2 alerts when merging 9bb0c271139f7660a76bd961ab354befd89b1f71 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

mfherbst commented 3 years ago

@maxscheurer Do you know how to exclude a file from LGTM. The siteconfig_example.py should be excluded from the analysis, I'd say.

maxscheurer commented 3 years ago

I think configuring LGTM is really tough, but maybe it works via a lgtm.yml config file?

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 8dab67d88436f148115e915b7902abeefff37c72 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 42126eacb08f8359469bac9449afe4bc9b121dc0 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 2 alerts when merging 6b40660c437400dea55f3f1bfb7ceabc4b5ab7f2 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 2 alerts when merging 00bf8224d8bef58294c47a3768dd2114a5149e04 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

mfherbst commented 3 years ago

I think configuring LGTM is really tough, but maybe it works via a lgtm.yml config file?

Could be, I can't be asked to crawl through the docs at the moment though. Would you be willing to take care of that. I tried the C++ thing a few days back, but it did not work out of the box so I have not yet done it either.

mfherbst commented 3 years ago

Ah ... ok that's surpirisingly easy.

maxscheurer commented 3 years ago

Ah ... ok that's surpirisingly easy.

I don't know if it works...

BTW did you test the siteconfig.py by specifying libtensor_autoinstall to some other path than the default? On my workstation the file is read, but the libtensor_autoinstall I set is being ignored.

mfherbst commented 3 years ago

I know of that problem. I tested it, but before a refactor that broke it. It's now fixed in the C++ coverage commit.

maxscheurer commented 3 years ago

I know of that problem. I tested it, but before a refactor that broke it. It's now fixed in the C++ coverage commit.

I fixed a typo in the setup, so it's working now 👍

maxscheurer commented 3 years ago

I bet LGTM for C++ will only work once this PR is merged into master, otherwise, I think there's no way to trigger another language on a PR...

mfherbst commented 3 years ago

I guess we do it later than.

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 1d04d782d25569dbf9644d24d01cd751bac9e7b3 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging f1a9857d6c78a209dc43fcc7e7fb40a4f3eaa615 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

maxscheurer commented 3 years ago

It seems like LGTM is just ignoring the .lgtm.yml file 😒 Hopefully it works when the PR is merged, but currently this feels like a waste of time.

mfherbst commented 3 years ago

Well now that I record C++ coverage the python coverage is ignored ^^.

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 59f687580052583622df5103ddd10090e2f8bf91 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 3f9e28ecc66d07f06bd18231202b591796394391 into 89472d0a86db0d5497c19d12149a6595b0a20175 - view on LGTM.com

new alerts:

maxscheurer commented 3 years ago

Question: Should we disable running the full test suite in conda builds? Instead, we could only run them during pip deployment and make the conda jobs depend on successful pip deployment...

Another suggestion is to just run tests on a single case in conda (e.g. h2o_sto3g) to make sure the built adcc in principle works.

If we support 4 Python versions (3.6-3.9, like Psi4) and 2 OS, we'd run the entire tests 8x during the deployment...

mfherbst commented 3 years ago

Another suggestion is to just run tests on a single case in conda (e.g. h2o_sto3g) to make sure the built adcc in principle works.

That plus the dependency on the pip job sounds like a good compromise. Also I suppose on the master / dev conda packages we can be more restrictive and e.g. exclude MacOS, which takes forever and perhaps some older python versions.

maxscheurer commented 3 years ago

we can be more restrictive and e.g. exclude MacOS, which takes forever and perhaps some older python versions.

You mean wrt the test runs?

mfherbst commented 3 years ago

No with the actual dev deployments.

mfherbst commented 3 years ago

How are the timings by the way?

maxscheurer commented 3 years ago

No with the actual dev deployments.

Okay, I don't have a good idea how to hack this in right now...

maxscheurer commented 3 years ago

How are the timings by the way?

Looking good, I'm running another large job right now, but I'd say we did not loose any performance on the way 🚀

maxscheurer commented 3 years ago

Okay, I've adapted the workflow for conda to only deploy py38 to dev. I'd like to give this maybe another try and see if it works..