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

Add Pytest & tests for sample factory #63

Merged
merged 17 commits into from
Nov 26, 2024
Merged

Add Pytest & tests for sample factory #63

merged 17 commits into from
Nov 26, 2024

Conversation

fabioseel
Copy link
Contributor

Adds pytest and some simple tests that check the sample factory integration.
Additional tests will likely be added to assert the functionality over the objective integration.

Moreover:

  • fixes a bug when trying to run rl simulations as the objective system is not yet working
  • now using sample factory from the official repository
  • fixes a bug when directory for scenarios has not yet been created
  • started moving to pathlib.Path for some files

@fabioseel
change: def file points to new sf repo
7b7f6b3
@fabioseel
doc: update doc in compile scenario
6f24fda
@fabioseel
chore: make to_sf_cfg static
9128475
@fabioseel
hotfix: make rl config without objective set runnable
6660db6
@fabioseel
feat: set train & wandb dirs for sample factory
932eeed
@fabioseel
feat: add simple tests for sample_factory with pytest
650ba69
@fabioseel
fix: relative path for configs should be relative to module
0e81445
@fabioseel
feat: add pytest and test to workflow
d59f12a
@fabioseel
fix: workflow for rl tests needs to build scenario first
53230c3
@fabioseel
fix: linting issues
0448df2
@fabioseel
fix: ensure out dir for scenario creation exists
6b4bb58
@fabioseel
fix: pass wandb usage option to sample_factory
4a6249f
@fabioseel
fix: ensure scenario dir exists. partial change to pathlib
be5b12b
@fabioseel
fix: lint issues, make code_check more expressive
a87b6e8
@fabioseel
chore: change sample factory reference to (currently) newest version …
3919e67 
…in official repo
@fabioseel fabioseel marked this pull request as ready for review November 20, 2024 13:46
@fabioseel
Copy link
Contributor Author

just for reference, this is a first step towards #49

@fabioseel fabioseel requested a review from alex404 November 22, 2024 09:14
@alex404
Copy link
Contributor

alex404 commented Nov 22, 2024

This all seems either (i) very straightforward or (ii) involves framework/rl stuff that I would need to look at more thoroughly.

Do you want me to review more thoroughly? If so it might not happen until monday. Otherwise it looks solid to me and I would merge

@fabioseel
Copy link
Contributor Author

No need to rush reviewing it, I'm working on a branch off of this atm anyway, so take your time
Just added another somewhat unrelated straightforward minor change :D

@alex404
Copy link
Contributor

alex404 commented Nov 22, 2024

Sounds good. I mean on my initial pass it looks good. Please re-request a review when you want me to have a more thorough look (or maybe I'll remember to do it on Monday).

alex404
alex404 reviewed Nov 22, 2024
View reviewed changes
Copy link
Contributor

@alex404 alex404 left a comment

Choose a reason for hiding this comment

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

Looks good so far. I should have a more thorough look at the big changes.

@fabioseel fabioseel requested a review from alex404 November 25, 2024 13:15
@alex404 alex404 merged commit e199a6f into master Nov 26, 2024
5 checks passed
@alex404 alex404 deleted the feat-add-pytest branch November 26, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex404 alex404 Awaiting requested review from alex404

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fabioseel @alex404