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.

Sign up for GitHub

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

Jump to bottom

718 integration test #158

Merged
merged 26 commits into from
Jan 27, 2025
Merged

718 integration test #158

merged 26 commits into from
Jan 27, 2025

Conversation

AntonZogk
Copy link
Collaborator

@AntonZogk AntonZogk commented Jan 24, 2025 •
edited by robertswh
Loading

Feat test main

Summary

Adding a testing for main.py , this is to check if all the methods are integrated together, also fixed some minor issues in main package which caused problems with main

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Checklists

This pull request meets the following requirements:

Creator Checklist

  • Installable with all dependencies recorded
  • Runs without error
  • Follows PEP8 and project-specific conventions
  • Appropriate use of comments, for example, no descriptive comments
  • Functions documented using Numpy style docstrings
  • Assumptions and decisions log considered and updated if appropriate
  • Unit tests have been updated to cover essential functionality for a reasonable range of inputs and conditions
  • Other forms of testing such as end-to-end and user-interface testing have been considered and updated as required

If you feel some of these conditions do not apply for this pull request, please
add a comment to explain why.

Reviewer Checklist

  • Test suite passes (locally as a minimum)
  • Peer reviewed with review recorded

Additional Information

Please provide any additional information or context that would help the reviewer understand the changes in this pull request.

Related Issues

Closes #149 , #81

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 27, 2025
View reviewed changes
.github/workflows/main.yaml Fixed Show resolved Hide resolved
AntonZogk
AntonZogk commented Jan 27, 2025
View reviewed changes
.github/workflows/main.yaml
- name: Install Python dependencies
run: |
python -m pip install --upgrade pip
pip install .[dev]
Copy link
Collaborator Author

@AntonZogk AntonZogk Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So this fails when forcing installation of rdsa-utils, the reason is because gssapi package is missing from the system (pythongssapi/requests-gssapi#14), so ubuntu-20.04 which hooks used to run on doesn't have gssapi so couldn't install rdsa utils.

Updated the hooks workflow with something more reproducible which doesn't require to install the package in dev mode anymore. We actually test building the package in 4 versions of CDP so not needed.

AntonZogk
AntonZogk commented Jan 27, 2025
View reviewed changes
setup.cfg Show resolved Hide resolved
@robertswh robertswh self-assigned this Jan 27, 2025
robertswh
robertswh reviewed Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@robertswh robertswh left a comment

Choose a reason for hiding this comment

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

Will go over this on a call

mbs_results/outputs/produce_additional_outputs.py Show resolved Hide resolved
mbs_results/config.json Show resolved Hide resolved
.github/workflows/main.yaml Fixed Show resolved Hide resolved
robertswh
robertswh approved these changes Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@robertswh robertswh left a comment

Choose a reason for hiding this comment

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

Talked through changes and added additional tasks where needed. Happy to merge

@AntonZogk AntonZogk merged commit c1b270b into main Jan 27, 2025
8 checks passed
@AntonZogk AntonZogk deleted the 718-Integration-test branch January 27, 2025 16:44
lhubbardONS pushed a commit that referenced this pull request Jan 28, 2025
@AntonZogk @robertswh @lhubbardONS
718 integration test (#158)
27793a5 
* Fix imports

* Fix multi args for strata

* Fix args in ratio of means and hard coded target

* Add missing args current, revision periods

* Remove referencename column

* Remove convert to str code

* Check for empty derived dfs

* Fix args for run_live_or_frozen call

* Add missing args for back data

* Add missing arg question_no

* Feat main testing

* Fix typo in cp path

* Force install rdsa-utils,boto,raz_client

* Use home directory as working directory for running test main

* Comment out rdsa,boto,raz

* Insstall rdsa utils, raz

* Update pre-commit hook job

* Install all  dependencies

* Fix trailing whitespace

* Update workflow, permissions and hooks commit hash

* try short hash

* full hash

* Update action hash

* Use v3.0.0 for hooks

* Use hash for hook action

* Run hooks

---------

Co-authored-by: Wil Roberts <[email protected]>
lhubbardONS added a commit that referenced this pull request Jan 28, 2025
@lhubbardONS @NathanKelly-ONS
@robertswh @Jday7879 @sarahcollyer @AntonZogk
701 gb cell numbers (#159)
1bbd7ea 
* Modify create_imputation_class and update test

* Creating convert_cell_number

* format to pass pre-commit

* 734 se filtering (#157)

* Add parameters for question number and period in selective editing output function

* Update method for loading mapping files. Changed form to formtype on doamin mapping file

* Fix CSV output by excluding index in additional outputs

* Remove CSV output of input data from selective editing contributor output

* Adding type hints and doctrings

* Derive auxiliary for selective editing (#139)

* added previous construction links for Q49, todo add aux var

* add function to calculate auxiliary variable

* add auxiliary value function and merge on output

* small corrections

* small fix

* formatting after pre-commits

* pass tests

* pass pre-commits

* Amend q49 to use construction link from current period

* Fix typos and incorrect values in unit test output data

* Updated:
- type issue for unit test
- Updated calculated value for reference 4 q49 to 625000

* pre commit hook changes

---------

Co-authored-by: Collyer <[email protected]>
Co-authored-by: Jordan Day <[email protected]>

* 718 integration test (#158)

* Fix imports

* Fix multi args for strata

* Fix args in ratio of means and hard coded target

* Add missing args current, revision periods

* Remove referencename column

* Remove convert to str code

* Check for empty derived dfs

* Fix args for run_live_or_frozen call

* Add missing args for back data

* Add missing arg question_no

* Feat main testing

* Fix typo in cp path

* Force install rdsa-utils,boto,raz_client

* Use home directory as working directory for running test main

* Comment out rdsa,boto,raz

* Insstall rdsa utils, raz

* Update pre-commit hook job

* Install all  dependencies

* Fix trailing whitespace

* Update workflow, permissions and hooks commit hash

* try short hash

* full hash

* Update action hash

* Use v3.0.0 for hooks

* Use hash for hook action

* Run hooks

---------

Co-authored-by: Wil Roberts <[email protected]>

* Adding convert_cell_number to estimation and small changes to convert_cell_number function

* Fixing derive_estimation_variables test

* pre-commit hooks

* Fixing pre processing estimation to include original cell number in output

* pre-commits

* pre-commits

---------

Co-authored-by: Nathan Kelly <[email protected]>
Co-authored-by: Wil Roberts <[email protected]>
Co-authored-by: NathanKelly-ONS <[email protected]>
Co-authored-by: Jordan-Day-ONS <[email protected]>
Co-authored-by: Wil Roberts <[email protected]>
Co-authored-by: Collyer <[email protected]>
Co-authored-by: Jordan Day <[email protected]>
Co-authored-by: Anton Zogkolli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertswh robertswh robertswh approved these changes

@Jday7879 Jday7879 Awaiting requested review from Jday7879 Jday7879 is a code owner

@giuliag92 giuliag92 Awaiting requested review from giuliag92 giuliag92 is a code owner

Assignees

@robertswh robertswh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Config missing needed values
2 participants
@AntonZogk @robertswh