marrlab / DomainLab

modular domain generalization: https://pypi.org/project/domainlab/
https://marrlab.github.io/DomainLab/
MIT License
42 stars 2 forks source link

cli to replace python main_out.py #740

Closed agisga closed 8 months ago

agisga commented 8 months ago

Addresses https://github.com/marrlab/DomainLab/issues/734

Seems to work fine. I have tested this commit as follows:

poetry build
pip install domainlab-0.4.3-py3-none-any.whl
domainlab -c ./examples/conf/vlcs_diva_mldg_dial.yaml
codecov-commenter commented 8 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d980abf) 95.12% compared to head (b8f9d63) 95.10%.

Files Patch % Lines
domainlab/cli.py 85.71% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #740 +/- ## ========================================== - Coverage 95.12% 95.10% -0.03% ========================================== Files 125 126 +1 Lines 4906 4920 +14 ========================================== + Hits 4667 4679 +12 - Misses 239 241 +2 ``` | [Flag](https://app.codecov.io/gh/marrlab/DomainLab/pull/740/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=marrlab) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/marrlab/DomainLab/pull/740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=marrlab) | `95.10% <85.71%> (-0.03%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=marrlab#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

smilesun commented 8 months ago

Addresses #734

Seems to work fine. I have tested this commit as follows:

poetry build
pip install domainlab-0.4.3-py3-none-any.whl
domainlab -c ./examples/conf/vlcs_diva_mldg_dial.yaml

nice work! Thanks! Is there a way also to let the pytest cover this new file? Since without testing it will drag the coverage to be below 95% @agisga

agisga commented 8 months ago

I'll look into how to include this in pytest.

agisga commented 8 months ago

@smilesun I have added some tests. At least on my machine they are passing.

smilesun commented 8 months ago

Do you receive email that CI is broken for this PR? @agisga

agisga commented 8 months ago

Do you receive email that CI is broken for this PR? @agisga

Yes, sorry. I think this is because I added the entrypoint via poetry, but CI installed domainlab from setup.py and so the entrypoint isn't created for the domainlab in CI. Should I try to change the CI workflow to use Poetry?

smilesun commented 8 months ago
=================================== FAILURES ===================================
_______________________________ test_entry_point _______________________________

self = [EntryPoint(name='markdown-it', value='markdown_it.cli.parse:main', group='console_scripts'), EntryPoint(name='tqdm', ...f2py.f2py2e:main', group='console_scripts'), EntryPoint(name='gdown', value='gdown.cli:main', group='console_scripts')]
name = 'domainlab'

    def __getitem__(self, name):  # -> EntryPoint:
        """
        Get the EntryPoint in self matching name.
        """
        if isinstance(name, int):
            warnings.warn(
                "Accessing entry points by index is deprecated. "
                "Cast to tuple if needed.",
                DeprecationWarning,
                stacklevel=2,
            )
            return super().__getitem__(name)
        try:
>           return next(iter(self.select(name=name)))
E           StopIteration

/opt/hostedtoolcache/Python/3.10.7/x[64](https://github.com/marrlab/DomainLab/actions/runs/7494347048/job/20402067119?pr=740#step:6:65)/lib/python3.10/importlib/metadata/__init__.py:350: StopIteration

During handling of the above exception, another exception occurred:

    def test_entry_point():
        eps = entry_points()
>       cli_entry = eps.select(group='console_scripts')['domainlab']

tests/test_cli.py:8: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = [EntryPoint(name='markdown-it', value='markdown_it.cli.parse:main', group='console_scripts'), EntryPoint(name='tqdm', ...f2py.f2py2e:main', group='console_scripts'), EntryPoint(name='gdown', value='gdown.cli:main', group='console_scripts')]
name = 'domainlab'

    def __getitem__(self, name):  # -> EntryPoint:
        """
        Get the EntryPoint in self matching name.
        """
        if isinstance(name, int):
            warnings.warn(
                "Accessing entry points by index is deprecated. "
                "Cast to tuple if needed.",
                DeprecationWarning,
                stacklevel=2,
            )
            return super().__getitem__(name)
        try:
            return next(iter(self.select(name=name)))
        except StopIteration:
>           raise KeyError(name)
E           KeyError: 'domainlab'

/opt/hostedtoolcache/Python/3.10.7/x64/lib/python3.10/importlib/metadata/__init__.py:352: KeyError
smilesun commented 8 months ago

Do you receive email that CI is broken for this PR? @agisga

Yes, sorry. I think this is because I added the entrypoint via poetry, but CI installed domainlab from setup.py and so the entrypoint isn't created for the domainlab in CI. Should I try to change the CI workflow to use Poetry?

then I do not have to at you each time. I posted the error above, what you suggested is related to https://github.com/marrlab/DomainLab/issues/739 if changing to poetry could resolve both issues then fine

smilesun commented 8 months ago

will this only be resolved on Thursday?

agisga commented 8 months ago

will this only be resolved on Thursday?

I'll try to get it done on Thursday. Sorry, I have a major deadline at work, so didn't really have time to look at GitHub. But tomorrow (Thursday) should be manageable.

agisga commented 8 months ago

@smilesun I fixed all checks here by having pytest executed through Poetry (see https://github.com/marrlab/DomainLab/pull/740/commits/230986665de908c4a6a73aed2009552fc92f1f97 for how Poetry was added to CI). Let me know if there are any other issues/questions.

smilesun commented 8 months ago

@agisga , cool, great work, thanks!