Skip to content

Testing (without holmes)#12

Merged
bradley-erickson merged 44 commits into
mainfrom
test-no-holmes
Jan 7, 2025
Merged

Testing (without holmes)#12
bradley-erickson merged 44 commits into
mainfrom
test-no-holmes

Conversation

@duckduckdoof
Copy link
Copy Markdown

Changes involve:

  • Moving outdated tests to an "old_tests" directory (we could just delete them)
  • Updating the main test test_awe_nlp_no_holmes.py to not include holmes
  • Various formatting changes for things like parserServer.py
  • Removing holmes as a dependency in the repo config
  • Adding a few new installation scripts.

Comment thread awe_workbench/web/oldParserServer.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this if we don't need it.

Comment thread tests/old_tests/__init__.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we need this entire directory?

Comment thread installation/install_template.sh Outdated
# AWE Workbench Install Script Template
# Author: Caleb Scott

# This script shows how to use the command-line arguments for the fresh_install.sh scripts.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The cases below could be further defined. I'm not really sure I follow the different cases. Additinoally, I see conda which is a little bit of a red-flag as the normal build process for the Learning Observer uses pip.

Comment thread installation/venv_fresh_install.sh Outdated

echo "============================ WARNING =============================="
echo "\nYou are about to install AWE on this system."
echo "\n* Ensure that you are using a python3.12 version."
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is Python 3.12 a requirement for this?

Comment thread installation/conda_fresh_install.sh Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file looks super similar to venv_fresh_install.sh. How do they differ from one another? How do I know which one to use? Do I have to use conda for installation?

Comment thread tests/old_tests/test_awe_nlp.py Outdated

Set of corresponding tests for document features found in awe_nlp.py of writingobserver.

Author: Caleb Scott (cwscott3@ncsu.edu)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All the other files in this directory are moved from the tests to here. I assume these are all to be deleted. Why does this file show changes instead of a move as well? Will we still be using this file?
The file being in old_tests makes me assume it should be deleted, which is causing some confusion.

-----------------------------------------------------------------------------------------
"""

# --- [ IMPORTS ] -----------------------------------------------------------------------
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not really the biggest fan of these types of comments. I understand that it helps break up the code a little bit, but especially in smaller files, I don't see the need for them. Even in large files, the code already does this to some degree (all the imports say import or from ... import, all the classes start with class, etc.).

@bradley-erickson
Copy link
Copy Markdown

bradley-erickson commented Nov 6, 2024

This needs documentation on how to install and run tests. Much of the original documentation is not very clean (no code-blocks for commands and a bunch of unneeded =s).

I tried to go and test this code to make sure it works on my machine as well and immediately ran into issues regarding installation. The requirements.txt file specifies awe_languagetool which I know where to find and install, but pip does not. For the additional AWE items, we should specify them similar to how its specified in Learning Observer.

AWE_SpellCorrect @ git+https://github.com/ETS-Next-Gen/AWE_SpellCorrect.git
AWE_Components @ git+https://github.com/ETS-Next-Gen/AWE_Components.git
AWE_Lexica @ git+https://github.com/ETS-Next-Gen/AWE_Lexica.git
AWE_LanguageTool @ git+https://github.com/ETS-Next-Gen/AWE_LanguageTool.git

@duckduckdoof
Copy link
Copy Markdown
Author

Yeah tbh I'm not sure why the requirements.txt file and setup.cfg are both there; the installation of Workbench doesn't explicitly refer to the former. Currently working to resolve those dependencies.

Additionally, a numpy update has stopped the python -m awe_workbench.setup.data from working... I've added a version lock and PR in AWE_Components (ArgLab/AWE_Components#6).

Comment thread setup.cfg
@bradley-erickson
Copy link
Copy Markdown

bradley-erickson commented Nov 18, 2024

Whenever I try to install the workbench, it winds up checking a bunch of different spacy versions. The download and installation of each one takes a bit (which heavily adds up over time). This should be investigated so the installation immediately installs an appropriate version.

@bradley-erickson
Copy link
Copy Markdown

@duckduckdoof any update on the spacy versioning? Do you also experience this when running pip install .?

@duckduckdoof
Copy link
Copy Markdown
Author

duckduckdoof commented Dec 6, 2024

@duckduckdoof any update on the spacy versioning? Do you also experience this when running pip install .?

I went ahead and tried a fresh install of workbench on a new python environment -- I only saw it attempt to install spacy3.8.2 first, then 3.5.4:

image

image

Of course, this may be quick because it's going for my cached versions? Should I elevate the verbosity to see if it's running through more than this?

@bradley-erickson
Copy link
Copy Markdown

I assume its also iterating through my cached versions of spacy to find one that works, but we ought to just have it install the one it needs from the get go.

Comment thread requirements.txt Outdated
aenum
statistics
srsly
awe_languagetool @ git+https://github.com/ArgLab/AWE_LanguageTool.git
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we need this file? Everything should probably be in the setup.cfg file instead for easier installation.

@bradley-erickson bradley-erickson merged commit a7a686b into main Jan 7, 2025
duckduckdoof added a commit that referenced this pull request Oct 15, 2025
* Added gitignore

* Removed typo

* Added missing test for lexical features

* consolidated AWE_Info declarations for better readability

* Added AWE_NLP tests from WO

* Fixed typo

* Added install script for conda

* Updated install script

* Updated install script

* Added protobuf patch

* Added baseline tests for WritingObserver

* Updated gitignore

* Added install scripts for conda and venv envs

* Cleaned install scripts

* Moved old tests, updated web files

* Updated main test to work without holmes

* Updated main test, added essay 'chooser'

* Updated essay 'chooser'

* Updates to parserServer

* Updated parserServer stubs & pipeline

* Removed coreferee from test; covered in pipeline.py

* No-holmes baseline tests

* Updated modules

* Removed holmes dependency

* Removed git conflict text

* Attempted removal of holmes from parserServer.py

* attempting removal of holmes from parserServer

* refactored to (mostly) use a local dict for document storage

* refactored to (mostly) use a local dict for document storage

* Update parserServer.py

* Update batch_summary.py

Added synchronization to batchSummary to make merging dataframes work right

* no meaningful changes, just removed unused imports

* Removed unecessary files

* Updated test name; requirements; docs

* Update old requirements

* Removed lexica to install; components already does this

* Removed old parser server code

* Update setup.cfg

removed branch designations

* Removed requirements in favor of setup.cfg

---------

Co-authored-by: Caleb Scott <goose@the-pond-11.lan>
Co-authored-by: cwscott3 <cwscott3@ncsu.edu>
Co-authored-by: arsalaan <arsalaankhan022@gmail.com>
Co-authored-by: askhan6 <askhan6@ncsu.edu>
Co-authored-by: ArsalaanK7 <132946619+ArsalaanK7@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.

4 participants