Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introducing the experimental package and refactoring test structure #433

Merged
merged 12 commits into from
Jan 22, 2025

Conversation

terrykong
Copy link
Collaborator

@terrykong terrykong commented Dec 6, 2024

What does this PR do ?

Introduces experimental directory structure for code and docs. As part of the restructuring, the tests are moved closer to the source to reduce the repetition of directory structure.

Before

NeMo-Aligner/
├── docs/
│   └── user-guide/
│       └── ppo.html
├── nemo_aligner/
│   ├── algorithms/
│   └── data/
│       └── datasets.py
└── tests/
    ├── test_datasets.py
    └── functional/
        └── test_cases/
            └── dpo-llama3

Proposal

NeMo-Aligner/
├── docs/
│   ├── user-guide/
│   │   └── ppo.html
│   └── user-guide-experimental/    <----- experimental docs
│       └── new-thing.html
├── nemo_aligner/
│   ├── algorithms/
│   ├── data/
│   │   ├── datasets.py
│   │   └── tests/
│   │       └── datasets_test.py
│   └── experimental/               <----- experimental sub-package
│       ├── <proj-name>/
│           ├── dataset.py          <----- experimental dataset
│           ├── new_algo.py         <----- experimental algo
│           ├── model.py            <----- experimental model
│           └── tests/
│               └── model_test.py   <----- experimental model test
└── tests/
    └── functional/
        └── dpo.sh
        └── test_cases/
            └── dpo-llama3
    └── functional_experimental/    <----- experimental functional tests (mirrors functional/ structure)
        ├── new_algo.sh
        └── test_cases/
            └── new_algo-llama3
  • By moving the docs out of the main dir, we can control what is rendered in the NeMo docs
  • colocating the tests in the same package makes it easier to find tests and reduces number of mirrored dirs
    • these can be filtered out when we publish to pypi (Needs more investigation)
  • functional tests will stay outside for convenience

@github-actions github-actions bot added the CI label Dec 6, 2024
@github-actions github-actions bot added Utils documentation Improvements or additions to documentation labels Jan 13, 2025
@terrykong terrykong added the Run CICD Set + un-set to retrigger label Jan 13, 2025
@terrykong terrykong changed the title Tk/experimental feat: introducing the experimental package and refactoring test structure Jan 14, 2025
@terrykong terrykong changed the title feat: introducing the experimental package and refactoring test structure feat: introducing the experimental package and refactoring test structure Jan 14, 2025
@github-actions github-actions bot removed the CI label Jan 14, 2025
@terrykong terrykong marked this pull request as ready for review January 14, 2025 07:42
@terrykong terrykong added Run CICD Set + un-set to retrigger and removed Run CICD Set + un-set to retrigger labels Jan 14, 2025
@terrykong
Copy link
Collaborator Author

@ko3n1g Tried excluding the test files via Manifest.in/exclude_package_data, but both don't seem to work when building a sdist wheel or simply pip install .

@terrykong terrykong added Run CICD Set + un-set to retrigger and removed Run CICD Set + un-set to retrigger labels Jan 16, 2025
@terrykong terrykong removed the request for review from gshennvm January 17, 2025 00:06
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, just a few questions below, and one suggestion: I think it'd be better to have tests of experimental features under a dedicated folder, so that we can easily identify them. Either under tests/functional/experimental, or tests/experimental, or tests/functional_experimental for instance.

Dockerfile Outdated Show resolved Hide resolved
nemo_aligner/experimental/README.md Outdated Show resolved Hide resolved
nemo_aligner/experimental/README.md Outdated Show resolved Hide resolved
nemo_aligner/utils/test_utils.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
@terrykong
Copy link
Collaborator Author

terrykong commented Jan 21, 2025

I think it'd be better to have tests of experimental features under a dedicated folder, so that we can easily identify them. Either under tests/functional/experimental, or tests/experimental, or tests/functional_experimental for instance

@odelalleau Makes sense! Please see 2893053

@terrykong terrykong added Run CICD Set + un-set to retrigger and removed Run CICD Set + un-set to retrigger labels Jan 21, 2025
odelalleau
odelalleau previously approved these changes Jan 21, 2025
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

@odelalleau Makes sense! Please see 2893053

Looking good, thanks!

@terrykong terrykong added Run CICD Set + un-set to retrigger and removed Run CICD Set + un-set to retrigger labels Jan 21, 2025
@terrykong terrykong enabled auto-merge (squash) January 21, 2025 19:13
Copy link
Collaborator

@ko3n1g ko3n1g left a comment

Choose a reason for hiding this comment

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

great change, thanks!

@terrykong terrykong merged commit 502ebde into main Jan 22, 2025
20 checks passed
@terrykong terrykong deleted the tk/experimental branch January 22, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Run CICD Set + un-set to retrigger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants