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

Changes based on reviews from Galaxy usegalaxy tools team #18

Merged
merged 7 commits into from
Jan 6, 2025

Conversation

paulocilasjr
Copy link
Contributor

Issues addressed:

  • Removed explicit overriding of $TMP* from the code.
  • Replaced all instances of pwd with .
  • Sanitized ${dataset.element_identifier}.
  • Incorporated yaml.safe_dump().
  • Changed format = "auto" in Ludwig_visualize.xml.
  • Replaced ludwig_model with the html format.
  • Added min and max for numeric parameters.
  • Fixed typo.

@qchiujunhao qchiujunhao changed the title Release v0.10 Changes based on reviews from Galaxy usegalaxy tools team Jan 4, 2025
tools/ludwig_autogenconfig.xml Outdated Show resolved Hide resolved
tools/ludwig_evaluate.xml Outdated Show resolved Hide resolved
tools/ludwig_evaluate.xml Outdated Show resolved Hide resolved
tools/ludwig_evaluate.xml Outdated Show resolved Hide resolved
tools/ludwig_experiment.xml Outdated Show resolved Hide resolved
tools/ludwig_train.xml Outdated Show resolved Hide resolved
tools/ludwig_train.xml Outdated Show resolved Hide resolved
tools/ludwig_train.xml Outdated Show resolved Hide resolved
tools/ludwig_train.xml Outdated Show resolved Hide resolved
tools/ludwig_visualize.xml Outdated Show resolved Hide resolved
@qchiujunhao
Copy link
Collaborator

Thank you, Paulo, for the changes—great work! Just a few adjustments are needed to pass the tests. Thank you!

@paulocilasjr
Copy link
Contributor Author

paulocilasjr commented Jan 4, 2025

How can I see the reason for the failure?

tools/ludwig_hyperopt.xml Show resolved Hide resolved
tools/ludwig_render_config.xml Show resolved Hide resolved
tools/ludwig_hyperopt.xml Show resolved Hide resolved
paulocilasjr and others added 2 commits January 5, 2025 11:16
Noticed that there is a comment about this.
Copy link
Collaborator

@qchiujunhao qchiujunhao left a comment

Choose a reason for hiding this comment

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

Passed all tests! Approved

@qchiujunhao qchiujunhao merged commit 6274302 into goeckslab:release_v0.10 Jan 6, 2025
11 checks passed
qchiujunhao added a commit that referenced this pull request Jan 7, 2025
* Removed torch and Horovod in Dockerfile

Since ludwig[full] will install torch and Ray. Ludwig has strong support for Ray, a framework for distributed computing, so we are not going to use Horovod. Ray doesn't support python 3.11 now.

* Upgrading version

* Updated version on macros.xml

* Auto generate a config from a dataset

* fixed new line issue for lint

* delete the code for config edit functionality

* updated the import for merge_with_defaults

* fixed a typo

* added a functionality - edit a config

* changed to not sort by keys

* removed render_config

* Replace chars back

* Removed this functionality

since Galaxy provides this functionality

* added a checked box for rendered config

* added a dropdown for selecting

the output feature

* updated to 0.10.3

* job_conf change (#12)

* job_conf change

* Readme comments resolved_1

* expect_num_outputs (#13)

* Readme comments and job_conf changes

* expect_num_outputs added (test on xml files)

* Gpu docker (#14)

* Update README.md

Specify that users should select the branch they want to install.

* path for job_conf.yml

* added a Dockerfile for GPU

* Reorganized the Dockerfile

* add unzip after testing

* define the docker requirements for gpu

* HTML & deploy (#16)

* lint and tests

* passed the tests

* deleted the code for debugging

* add a triggered branch

* change to new version of actions

* lint the import order

* new html and assert_contents

* update hyperopt test file

* define the order and replace pwd

* passed all tests locally

* use the same docker image

* Update temp_hyperopt_training.learning_rate.pdf

after update_test_data

* Removed tool declarations in actions.

* Delete slash.yaml (#19)

* Changes based on reviews from Galaxy usegalaxy tools team  (#18)

* comments fixed

* Update ludwig_hyperopt.py

line added for lint

* fix_from_comments_Junhao

* fix_#2

* Change discover_datasets pattern in viz outputs

Noticed that there is a comment about this.

---------

Co-authored-by: JunhaoQiu <56094690+qchiujunhao@users.noreply.github.com>

* fixed issues after testing on the cancer server  (#20)

* fixed issues after testing on the cancer server

1. The Ray server requires `TMP` environment variables to start.

2. Sometimes datasets have spaces at the end of their filenames. During sanitization, these spaces cause the name to become `**.csv_`, which raises an issue. To fix this, spaces in the filename are replaced with nothing before sanitization.

3.Removed the table in the hyperopt HTML report since it was too visually unappealing. A JSON file output is provided instead.

* test data updated

* python lint

* replace -> strip

* Outputs changes (#21)

* deleted table from html

1. deleted test stats table from experiment report html
2. change some output name

* python lint

---------

Co-authored-by: Paulo Cilas Morais Lyra Junior <58008200+paulocilasjr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants