Skip to content

Have CI use mamba#2983

Merged
IAlibay merged 2 commits intodevelopfrom
ci-mamba
Oct 15, 2020
Merged

Have CI use mamba#2983
IAlibay merged 2 commits intodevelopfrom
ci-mamba

Conversation

@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Oct 13, 2020

mamba is a drop-in replacement for the conda CLI but faster

Hopefully, this will make CI goes faster by saving on install time.

PR Checklist

  • [ ] Tests?
  • [ ] Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

mambda is a dropin replacement for the conda CLI but faster
@IAlibay
Copy link
Member

IAlibay commented Oct 13, 2020

Test failure might be related to #2958 ? It does look like switching to mamba shaves off a couple of minute from build time.

@jbarnoud
Copy link
Contributor Author

I am going to restart the failing test. For the record, here is the error we had:

_______________ TestRDKitConverter.test_monomer_info[resid 1-0] ________________

self = <MDAnalysisTests.coordinates.test_rdkit.TestRDKitConverter object at 0x7f2d5d0a4748>

pdb = <Universe with 1877 atoms>, sel_str = 'resid 1', atom_index = 0

    @pytest.mark.parametrize("sel_str, atom_index", [

        ("resid 1", 0),

        ("resname LYS and name NZ", 1),

        ("resid 34 and altloc B", 2),

    ])

    def test_monomer_info(self, pdb, sel_str, atom_index):

        sel = pdb.select_atoms(sel_str)

>       umol = sel.convert_to("RDKIT")

testsuite/MDAnalysisTests/coordinates/test_rdkit.py:196: 

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

package/MDAnalysis/core/groups.py:3146: in convert_to

    return converter().convert(self.atoms)

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <RDKitConverter>, obj = <AtomGroup with 14 atoms>, cache = True

NoImplicit = True, max_iter = 200

    def convert(self, obj, cache=True, NoImplicit=True, max_iter=200):

        """Write selection at current trajectory frame to

        :class:`~rdkit.Chem.rdchem.Mol`.

    

        Parameters

        -----------

        obj : :class:`~MDAnalysis.core.groups.AtomGroup` or :class:`~MDAnalysis.core.universe.Universe`

    

        cache : bool

            Use a cached copy of the molecule's topology when available. To be

            used, the cached molecule and the new one have to be made from the

            same AtomGroup object (same id) and with the same arguments passed

            to the converter (with the exception of this `cache` argument)

        NoImplicit : bool

            Prevent adding hydrogens to the molecule

        max_iter : int

            Maximum number of iterations to standardize conjugated systems.

            See :func:`_rebuild_conjugated_bonds`

        """

        # parameters passed to atomgroup_to_mol and used by the cache

        kwargs = dict(NoImplicit=NoImplicit, max_iter=max_iter)

    

        try:

            from rdkit import Chem

        except ImportError:

            raise ImportError("RDKit is required for the RDKitConverter but "

                              "it's not installed. Try installing it with \n"

                              "conda install -c conda-forge rdkit")

        try:

            # make sure to use atoms (Issue 46)

            ag = obj.atoms

        except AttributeError:

            raise TypeError("No `atoms` attribute in object of type {}, "

                            "please use a valid AtomGroup or Universe".format(

                                type(obj))) from None

    

        if cache:

            # key used to search the cache

            key = f"<{id(ag):#x}>" + ",".join(f"{key}={value}"

                                            for key, value in kwargs.items())

            try:

                mol = self._cache[key]

            except KeyError:

                # only keep the current molecule in cache

                self._cache.clear()

                # create the topology

                self._cache[key] = mol = self.atomgroup_to_mol(ag, **kwargs)

            # continue on copy of the cached molecule

            mol = copy.deepcopy(mol)

        else:

            self._cache.clear()

            mol = self.atomgroup_to_mol(ag, **kwargs)

    

        # add a conformer for the current Timestep

        if hasattr(ag, "positions"):

            if np.isnan(ag.positions).any():

                warnings.warn("NaN detected in coordinates, the output "

                              "molecule will not have 3D coordinates assigned")

            else:

                # assign coordinates

                conf = Chem.Conformer(mol.GetNumAtoms())

                for atom in mol.GetAtoms():

                    idx = atom.GetIntProp("_MDAnalysis_index")

>                   xyz = ag.positions[idx].astype(float)

E                   IndexError: index 14 is out of bounds for axis 0 with size 14

package/MDAnalysis/coordinates/RDKit.py:333: IndexError

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #2983 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2983   +/-   ##
========================================
  Coverage    93.05%   93.05%           
========================================
  Files          186      186           
  Lines        24609    24609           
  Branches      3187     3187           
========================================
  Hits         22900    22900           
  Misses        1661     1661           
  Partials        48       48           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9543ba...bc364bc. Read the comment docs.

@jbarnoud
Copy link
Contributor Author

The real gain will be when ci-helpers will make micromamba available (astropy/ci-helpers#458). For now, the environment is still created using conda.

@jbarnoud jbarnoud marked this pull request as ready for review October 14, 2020 08:12
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

I don't have any experience with mamba, so just in case there's someone with strong opinions on this switch (@MDAnalysis/coredevs ?) I'll leave it for ~ 24h and merge tomorrow.

@IAlibay IAlibay merged commit 2ee4e9d into develop Oct 15, 2020
@IAlibay IAlibay deleted the ci-mamba branch October 15, 2020 13:56
orbeckst pushed a commit that referenced this pull request Oct 24, 2020
Not an issue targetted fix.
Improve CI (no user facing changes)

- Overview

  mamba is a dropin replacement for the conda CLI but faster

- Work done in this PR

  Enabled the `MAMBA` environment variable keyword for the CI helper.

(cherry picked from commit 2ee4e9d)
@orbeckst
Copy link
Member

I backported mamba to 1.0.1 #2768 in 73cd1e6

lilyminium pushed a commit to lilyminium/mdanalysis that referenced this pull request Dec 9, 2020
Not an issue targetted fix.
Improve CI (no user facing changes)

- Overview

  mamba is a dropin replacement for the conda CLI but faster

- Work done in this PR

  Enabled the `MAMBA` environment variable keyword for the CI helper.

(cherry picked from commit 2ee4e9d)
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
Not an issue targetted fix.

## Overview

mamba is a dropin replacement for the conda CLI but faster

## Work done in this PR

Enabled the `MAMBA` environment variable keyword for the CI helper.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants